Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Basic structure for development of OpenAPI parser #2

Closed
wants to merge 76 commits into from
Closed

Basic structure for development of OpenAPI parser #2

wants to merge 76 commits into from

Conversation

de-sh
Copy link
Contributor

@de-sh de-sh commented Jan 9, 2019

Contains only files necessary to build an OpenAPI parser moved from within hydrus.
NOTE: Hydrus is not included in requirements.txt. Should it be?

@chrizandr
Copy link
Member

chrizandr commented Jan 9, 2019

Not sure what you mean by a mono repo.

Big repos that all necessary components within it, making code DRY, but extremely large bundles...
https://en.wikipedia.org/wiki/Monorepo

Okay. I don't think it's not a good idea to bundle them in one repo. The parser is still in the early stages, a lot is left to be added to it. A lot more work will be done and it will have use cases where hydrus is not needed.

What about DRY principal then?

Agreed that this will be violated. Maybe consider moving the hydraspec module out of hydrus. Since they are mostly tools that can be used even without hydrus.

We might need more discussion on this.

@shravandoda
Copy link

Was anyone able to solve the issue without making the PyPi package?
I couldn't find a way to do it with github. Pip cannot install egg files

@de-sh
Copy link
Contributor Author

de-sh commented Jan 9, 2019

Yeah, I used a similar trick as yours, but I used this file by @xadahiya as inspiration 😃

It needs to be a chnage in the CI config file.

@Mec-iS
Copy link
Contributor

Mec-iS commented Jan 9, 2019

I suppose we should re-organize the code so that hydrus can use the parser as an external library.
Segregation of concerns is evidently not in place if there is so much entanglement between hydrus and the parser.

I have opened #5 as reference issue.

@shravandoda
Copy link

Yeah, I used a similar trick as yours, but I used this file by @xadahiya as inspiration

It needs to be a chnage in the CI config file.

Nice!

@chrizandr
Copy link
Member

I've opened a develop branch in the repo, please create all PRs to develop.

@chrizandr chrizandr closed this Jan 9, 2019
@de-sh de-sh changed the base branch from master to develop January 9, 2019 17:14
@de-sh
Copy link
Contributor Author

de-sh commented Jan 9, 2019

I guess we can reopen this for further discussion as this is now in develop base...

@chrizandr chrizandr reopened this Jan 9, 2019
@Mec-iS
Copy link
Contributor

Mec-iS commented Jan 19, 2019

as per hydra-python-core, this repository has to preserve the history of commits from hydrus

@Mec-iS
Copy link
Contributor

Mec-iS commented Jan 19, 2019

Contains only files necessary to build an OpenAPI parser moved from within hydrus.
NOTE: Hydrus is not included in requirements.txt. Should it be?

Only hydrus should import hydra-openapi-parser. In no way the parser should import from hydrus.
The parser should have its own CLI, called from hydrus when it is available, otherwise the parser should be usable also with its own CLI.

@de-sh
Copy link
Contributor Author

de-sh commented Jan 20, 2019

How must I refactor the test_parser.py to reflect that the absolute path is not from a hydrus module but the openapi_parser module?

@Mec-iS
Copy link
Contributor

Mec-iS commented Jan 21, 2019

Use os.path.dir to find the right directory from the module's starting directory (you can use abs(__file__)).

Point the test files to the files within parent directory of the tests to solve the issue with os.path. This must be tested on non unix OSes
@de-sh
Copy link
Contributor Author

de-sh commented Jan 21, 2019

I had some issues with Module not found: hydra-openapi-parser but I solved by instead using __file__../ effectively redirecting to the parent directory, as was required to be done.

@Mec-iS
Copy link
Contributor

Mec-iS commented Feb 22, 2019

@de-sh Please open a new PR on the newly imported code at df27d30

@Mec-iS Mec-iS closed this Feb 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants