openapi-python-client icon indicating copy to clipboard operation
openapi-python-client copied to clipboard

feat: Support for recursive and circular references using lazy imports

Open mtovt opened this issue 3 years ago • 5 comments

Continuation of #582. Seems, closes: #338, #466 Adds lazy imports to avoid circular import errors, which #582 has. Not fully finished. I’d like to polish it after receiving feedback.

Instead of #636

todo:

  • [ ] Remove useless pass, which sometimes appears in to_dict method of model.
  • [x] Make type checking of generated client pass (Seems a type in quotes without import does not satisfy Mypy. Seems may be fixed by using TypeVar)
  • [x] Add test related to quoted argument if this approach will be used.
  • [ ] Do not use lazy imports if a model does not spawn circular import error (I’d say it separate PR)

mtovt avatar Sep 13 '22 22:09 mtovt

Codecov Report

Merging #670 (42a56da) into main (47e576c) will not change coverage. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              main      #670    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           49        49            
  Lines         1800      1946   +146     
==========================================
+ Hits          1800      1946   +146     
Impacted Files Coverage Δ
openapi_python_client/parser/openapi.py 100.00% <100.00%> (ø)
...penapi_python_client/parser/properties/__init__.py 100.00% <100.00%> (ø)
...i_python_client/parser/properties/enum_property.py 100.00% <100.00%> (ø)
..._python_client/parser/properties/model_property.py 100.00% <100.00%> (ø)
...penapi_python_client/parser/properties/property.py 100.00% <100.00%> (ø)
openapi_python_client/parser/properties/schemas.py 100.00% <100.00%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

codecov[bot] avatar Sep 18 '22 17:09 codecov[bot]

Thanks for continuing this journey—this is the most challenging problem that's been solved for this project in a long time 😁. This approach looks good—it's complex, but this is the cleanest solution I've seen so far, and I think about as clean as it will ever get. Well done!

I solved the mypy issue, and I think that checks are passing now (though GitHub is acting weird for me).

🥳 🎉 🙇

dbanty avatar Sep 18 '22 17:09 dbanty

I figured out where that pass is coming from but haven't fixed it yet. All of the types used in annotations are imported again, but some of them are not required to convert the type to a dict (e.g., ModelWithCircularRefInAdditionalPropertiesA). In this case, when autoflake runs and deletes unused imports, it leaves a pass statement when all imports were deleted (for some reason).

Not sure how to solve this without adding more complexity to selecting imports.

dbanty avatar Sep 27 '22 00:09 dbanty

@dbanty Thank you so much for the fast feedback and your help!

Seems all required tests were added.

autoflake currently depends on temporarily replacing unused imports/variables with pass and then removing the useless ones in a second pass.

Seems autoflake has had this issue for several years. https://github.com/PyCQA/autoflake/issues/52 https://github.com/PyCQA/autoflake/issues/65

I suggest fixing it later since it doesn’t produce any troubles + there are several points to improve and may be better to do it centralized.

Checks now are failing because of poetry export -f requirements.txt | poetry run safety check --bare --stdin\ command. Have not investigated yet.

mtovt avatar Oct 02 '22 04:10 mtovt

Looks like the locked version of pydantic has a vulnerability, poetry update pydantic should fix it. I need to find out why Renovate isn't auto updating the lock file anymore 🤔

dbanty avatar Oct 02 '22 04:10 dbanty

Hello! Seems the review is required here to be merged. I didn't see how to request it explicitly. Just let me know if I should do something here.

mtovt avatar Oct 17 '22 13:10 mtovt

Yup, I think this is good to go, I’ve just been traveling pretty much nonstop since the last update 😅. This will make it into the next release though! 🎉

dbanty avatar Oct 17 '22 15:10 dbanty