feat: templatize api, endpoint init files #403

p1-ra · 2021-04-30T14:29:34Z

This PR add the api and endpoint init.py file as template. I had specifics needs for it, may be useful for others.

BR.

codecov · 2021-04-30T14:35:13Z

Codecov Report

Merging #403 (ae18d84) into main (f1caadc) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #403   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           47        47           
  Lines         1558      1563    +5     
=========================================
+ Hits          1558      1563    +5

Impacted Files	Coverage Δ
openapi_python_client/__init__.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f1caadc...ae18d84. Read the comment docs.

dbanty · 2021-04-30T19:04:12Z

openapi_python_client/__init__.py

+                package_name=self.package_name,
+                endpoint_collections_by_tag=endpoint_collections_by_tag,


I assume you're passing these params because you need them, but for a developer working on just openapi-python-client it looks like these are not needed and might get removed at some point. I suggest either adding a note here or, better yet, a custom template in the end to end tests which uses them so we can see why they're there.

Good catch. I followed your recommendation:

eda38ed Add new custom template tests, I had to change a little bit how the expected diff was handled:

Instead of a dict of {filename: content} it now use a dict of {file path: content}, to be able to handle files that have the same name but are defined at different paths

Since I've added a custom template sample, directly hardcoding the expected content seems to become a bad idea. That's why I propose to introduce the custom-templates-golden-record which contains all expected diff when using custom templates

96fcdd6 add the custom-templates-golden-record generation to the regen task

We used to generate a complete golden-record for custom to compare as well but code reviews got really burdensome since the same content would effectively change twice and it didn't add much value. At a glance, it looks like you've set it up to only generate the specific files we're interested in so that should be a good middleground between the two approaches (generate everything vs generate nothing). I'll take a deeper look later on, thanks!

barjomet · 2021-05-05T14:29:17Z

end_to_end_tests/test_custom_templates/endpoint_init.py.jinja

+{%- for part in name -%}
+{{ '_' if not loop.first and part in 'ABCDEFGHIJKLMNOPQRSTUVWXYZ' and (loop.index - last_snakize.at) > 1 and not last_snakize.update({'at': loop.index}) }}{{ part | lower }}
+{%- endfor -%}
+{% endmacro %}


this macro will render wrong names module__enpoint_func with extra underscore.
@dbanty whouldn't it be nice to expose utils methods to jinja templates?

by just adding to Project class

self.env.globals.update(snake_case=snake_case, pascal_case=pascal_case)

Probably? I don't use custom templates at all and they aren't considered stable, I just mostly approve other folks' contributions for now so they can do what they need. If having those available in custom templates would be useful to you then we can totally pass them through. I think in the primary templates though we don't do any string transformations, it's all done in the Python parsing code.

I agree, it will be nice to have, best I thinks will be to make https://github.com/triaxtec/openapi-python-client/blob/main/openapi_python_client/utils.py accessible to jinja.

I've introduce the change in 95df40f and updated the custom template sample in b9a67e1

tsotnikov

@p1-ra very good job!

Please take a look at #431, (my bad 😅, I did not notice this PR and tried to implement same thing) maybe some test ideas could be useful, like this one:

dircmp already does cmpfiles internally and stores the same result in diff_files and funny_files. Reference: https://github.com/python/cpython/blob/main/Lib/filecmp.py#L186
So it just need some parsing to retrieve the result recursively from the subdirectories.

tsotnikov · 2021-05-19T21:30:38Z

openapi_python_client/__init__.py

+            endpoint_init_path = tag_dir / "__init__.py"
+            endpoint_init_template = self.env.get_template("endpoint_init.py.jinja")
+            endpoint_init_path.write_text(
+                endpoint_init_template.render(package_name=self.package_name, tag=tag, endpoints=collection.endpoints),


EndpointCollection does already have a tag attribute so it would be cleaner to pass the object:

Suggested change

endpoint_init_template.render(package_name=self.package_name, tag=tag, endpoints=collection.endpoints),

endpoint_init_template.render(package_name=self.package_name, endpoint_collection=collection),

And about the package_name, probably not really needed as a relative import always can be done. If we do pass it, maybe we would also need to pass it also to endpoint_module.py.jinja template also for consistency (Note that I am not against passing it, just a thought) (Edit: nevermind, I see in the examples that you use it for other purposes in addition to imports, like for generating a class name)

Agreed resolved in cdd304c

Regarding the package_name I do have specific needs for it, so definitly prefer to keep it indeed 😁
I've made a change, package metadata are now wildly available in all templates 77acd25

Please review other template rendering in this file where package_name is passed.
Also I see some renders with description and version instead of package_description and package_version so that could be unified.

Indeed, normalized on d1120f2

I've also wildly spread the project metadata vars to all template

@dbanty what do you thinks about it? We are including a breaking change here by renaming {version, description} to {package_version, package_description}.

Imo, package_X kind of make more sense if we spread them accross all templates

The template API is not stable, so I don't care too much about breaking changes to it 😅. So I'm good with adding some clarity / consistency.

tsotnikov · 2021-05-19T21:45:24Z

openapi_python_client/__init__.py

+        api_init_path.write_text(
+            api_init_template.render(
+                package_name=self.package_name,
+                endpoint_collections_by_tag=endpoint_collections_by_tag,


I would suggest passing endpoint_collections_by_tag in the dictionary form instead of a list (after doing .items()) for two reasons:

Naming may be confusing as self.openapi.endpoint_collections_by_tag is dictionary but in the template endpoint_collections_by_tag is a list.

It is more flexible and allows easily looping on unique tags

Agreed, resolved in cdd304c

tsotnikov · 2021-05-19T22:19:35Z

end_to_end_tests/test_end_to_end.py

@@ -12,7 +12,10 @@
 def _compare_directories(
    record: Path,
    test_subject: Path,
-    expected_differences: Optional[Dict[str, str]] = None,
+    expected_differences: Optional[
+        Dict[str, str]


I think this should be Optional[Dict[Path,str]]

Indeed, resolved in b541e01

dbanty

Awesome, great work! Glad to see so much thought being put into the templates by the community 💜. I have a couple suggestions, mostly around making the e2e tests a bit cleaner (IMO). I tested locally and my changes all passed, but I could have messed something up pasting into GitHub so let me know if you have any issues (or fundamentally disagree with the changes). Thank you!!

end_to_end_tests/test_end_to_end.py

openapi_python_client/__init__.py

@ramnes

…which will cause some to be renamed [openapi-generators#413][openapi-generators#432]. Thanks @ramnes!

@csymeonides-mf

…r complex types (openapi-generators#372). Thanks @csymeonides-mf! * fix: Detect File fields correctly * fix: Optional File field caused missing import error * fix: Non-string multipart fields must be stringified * chore: Reformat * fix: Put non-file fields as tuples in the files dict * refactor: Avoid runtime type checks * fix: Declaring types is necessary * fix: Remove dead code * fix: Non-JSON non-file multipart props need to be text/plain * refactor: Avoid breaking change Co-authored-by: Constantinos Symeonides <[email protected]>

… tests

… imprv Co-authored-by: Dylan Anthony <[email protected]>

p1-ra · 2021-06-09T14:16:44Z

I've merged your suggestions and rebased on top of current main branch @dbanty

dbanty · 2021-06-28T00:17:39Z

@p1-ra I had to rebase on main again because of some history changes with the 0.9.2 release. So I copied this to #442 and merged, thanks!

p1-ra force-pushed the feat/templatize-api-endpoint-init-files branch from 4093e72 to e0e8302 Compare April 30, 2021 14:33

dbanty requested changes Apr 30, 2021

View reviewed changes

p1-ra force-pushed the feat/templatize-api-endpoint-init-files branch 2 times, most recently from b7de1ed to 96fcdd6 Compare May 3, 2021 14:36

barjomet reviewed May 5, 2021

View reviewed changes

p1-ra force-pushed the feat/templatize-api-endpoint-init-files branch 3 times, most recently from 9a5cf23 to 2715dbf Compare May 6, 2021 07:38

tsotnikov mentioned this pull request May 19, 2021

feat: Add custom templates for api init files #431

Closed

tsotnikov reviewed May 19, 2021

View reviewed changes

p1-ra force-pushed the feat/templatize-api-endpoint-init-files branch 2 times, most recently from 77acd25 to d1120f2 Compare May 20, 2021 12:15

tsotnikov previously approved these changes May 20, 2021

View reviewed changes

dbanty requested changes May 24, 2021

View reviewed changes

dbanty added this to the 0.10.0 milestone May 24, 2021

ramnes and others added 2 commits June 2, 2021 08:17

fix(generator)!: Improved the consistency of snake_cased identifiers …

56198ee

…which will cause some to be renamed [openapi-generators#413][openapi-generators#432]. Thanks @ramnes!

p1-ra dismissed stale reviews from tsotnikov via b2f3891 June 9, 2021 13:58

Nementon and others added 10 commits June 9, 2021 16:06

templatize api,endpoint init files

a0126cc

e2e / add {api_init.py.jina, endpoint_init.py.jinja} custom templates…

35268ae

… tests

expose utils module to jinja2

586c658

test_custom_templates / replace jinja2 marcro with call to utils module

92499e1

task / regen / regen custom template golden record

9c6a0f6

test_end_to_end / correct typing

370eba1

endoint __init__ template / factorize propagated vars

93a14c7

templates / globaly propagate package metadata vars

20cd818

test___init__.py / correct templates API call breaking changes

8f51984

Update test_end_to_end.py,openapi_python_client/__init__.py : various…

20986ac

… imprv Co-authored-by: Dylan Anthony <[email protected]>

regen templates golden records (rebased on main)

ae18d84

p1-ra force-pushed the feat/templatize-api-endpoint-init-files branch from d3ee968 to ae18d84 Compare June 9, 2021 14:14

p1-ra requested a review from dbanty June 9, 2021 14:16

dbanty force-pushed the main branch from f1caadc to 1e3b165 Compare June 14, 2021 00:35

dbanty mentioned this pull request Jun 28, 2021

feat: Allow custom templates for API and endpoint __init__ files. #442

Merged

dbanty closed this Jun 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: templatize api, endpoint init files #403

feat: templatize api, endpoint init files #403

p1-ra commented Apr 30, 2021

codecov bot commented Apr 30, 2021 •

edited

Loading

dbanty Apr 30, 2021

p1-ra May 3, 2021 •

edited

Loading

dbanty May 3, 2021

barjomet May 5, 2021

dbanty May 5, 2021

p1-ra May 5, 2021

p1-ra May 6, 2021 •

edited

Loading

tsotnikov left a comment •

edited

Loading

tsotnikov May 19, 2021 •

edited

Loading

p1-ra May 20, 2021

tsotnikov May 20, 2021

p1-ra May 20, 2021

p1-ra May 20, 2021 •

edited

Loading

dbanty May 24, 2021

tsotnikov May 19, 2021 •

edited

Loading

p1-ra May 20, 2021

tsotnikov May 19, 2021

p1-ra May 20, 2021

dbanty left a comment

p1-ra commented Jun 9, 2021

dbanty commented Jun 28, 2021

		package_name=self.package_name,
		endpoint_collections_by_tag=endpoint_collections_by_tag,

	endpoint_init_template.render(package_name=self.package_name, tag=tag, endpoints=collection.endpoints),
	endpoint_init_template.render(package_name=self.package_name, endpoint_collection=collection),

feat: templatize api, endpoint init files #403

feat: templatize api, endpoint init files #403

Conversation

p1-ra commented Apr 30, 2021

codecov bot commented Apr 30, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

p1-ra May 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p1-ra May 6, 2021 • edited Loading

Choose a reason for hiding this comment

tsotnikov left a comment • edited Loading

Choose a reason for hiding this comment

tsotnikov May 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p1-ra May 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsotnikov May 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbanty left a comment

Choose a reason for hiding this comment

p1-ra commented Jun 9, 2021

dbanty commented Jun 28, 2021

codecov bot commented Apr 30, 2021 •

edited

Loading

p1-ra May 3, 2021 •

edited

Loading

p1-ra May 6, 2021 •

edited

Loading

tsotnikov left a comment •

edited

Loading

tsotnikov May 19, 2021 •

edited

Loading

p1-ra May 20, 2021 •

edited

Loading

tsotnikov May 19, 2021 •

edited

Loading