HTTP-APIs / hydra-openapi-parser

OpenAPI to Hydra parser
MIT License
9 stars 9 forks source link

Basic structure for development of OpenAPI parser #2

Closed de-sh closed 5 years ago

de-sh commented 5 years ago

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 commented 5 years ago

Since imports are from hydrus it should be added to the requirements

de-sh commented 5 years ago

Ok, up to it! @chrizandr should I also add the tests from hydrus/tests/test_parser.py?

xadahiya commented 5 years ago

hydrus CLI and a couple of other files need the parser as a dependency and parser needs hydrus as a dependency. If we create separate packages won't it create circular dependency error?

de-sh commented 5 years ago

won't it create circular dependency error?

Yeah, a mono repo is more obviously better here... but, don't take my word for it...

chrizandr commented 5 years ago

Not sure what you mean by a mono repo.

Since we are only using doc_writer from hydrus, maybe create a copy of the doc_writer here?

xadahiya commented 5 years ago

What about DRY principal then?

de-sh commented 5 years ago

Not sure what you mean by a mono repo.

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

chrizandr commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

de-sh commented 5 years ago

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

Mec-iS commented 5 years ago

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

Mec-iS commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

de-sh commented 5 years ago

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 commented 5 years ago

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