Don't depend on stringcase (closes #369) #375

ramnes · 2021-04-01T15:17:02Z

Let's reimplement what we need here so that we don't depend on stringcase
anymore (see #369 for rationale). I've added a few test cases for stuff I've
seen break along the way.

Let's reimplement what we need here so that we don't depend on stringcase anymore (see openapi-generators#369 for rationale). I've added a few test cases for stuff I've seen break along the way.

ramnes · 2021-04-01T15:19:09Z

This kind of work with regexps and a large variety of use cases is always a bit fragile, so even if all the tests pass, I'd recommend that you test the branch against other openapi.yaml files you may have already a generated client to see if there isn't any weird diff. Only fixed a lot of bugs on my side. :)

dbanty · 2021-04-01T15:35:30Z

Is there a rationale for implementing our own vs relying on one of the other libraries you mentioned in #369? I'm thinking one of them might have a more expansive test suite to catch any corner-case bugs (and if they don't... they should 😂)

ramnes · 2021-04-01T18:42:21Z

Yeah, none of the two libraries passed the tests out of the box. Both needed hacks or pull-requests to make everything work together. I figured out that rather than spending time to write hacks or modify yet another project, I'd rather implement that here. It's just three functions to maintain, so I don't think it's worth a dependency, and most importantly, that would give you full control over the generated output.

About full control... The e2e tests are broken as you can see, and this is due to what seems to me to be an inconsistency in the previous implementation. :)

What do you think of such diffs in the generated output?

-from ...models.test_inline_objects_response_200 import TestInlineObjectsResponse_200
+from ...models.test_inline_objects_response_200 import TestInlineObjectsResponse200

-from .model_with_union_property_inlined_fruit_type0 import ModelWithUnionPropertyInlinedFruitType0
-from .model_with_union_property_inlined_fruit_type1 import ModelWithUnionPropertyInlinedFruitType1
+from .model_with_union_property_inlined_fruit_type_0 import ModelWithUnionPropertyInlinedFruitType0
+from .model_with_union_property_inlined_fruit_type_1 import ModelWithUnionPropertyInlinedFruitType1

ramnes · 2021-04-01T18:58:28Z

Here are two diffs that would make things more consistent: https://gist.github.com/ramnes/611b874b878f55f5bfc4b781d48c40a1

current.diff is the diff generated by the current state of the PR, and next.diff is the diff generated by a one line diff I have not pushed yet. In my opinion next.diff looks much better, but it has a higher chance of introducing breaking changes for people that already have a generated client and want to update the generator.

ramnes · 2021-04-01T19:16:00Z

FWIW, here is the diff when running the current version of the PR against sftpgo-client: ramnes/sftpgo-client@8e620d5

dbanty

A couple tweaks to make, and I think I'm going to try and preserve the previous behavior of saving leading delimiters just to reduce update friction.

dbanty · 2021-04-25T18:43:48Z

openapi_python_client/utils.py

-    return fix_keywords(stringcase.pascalcase(sanitize(value.replace(" ", ""))))
+    words = split_words(sanitize(value))
+    words = [word.capitalize() if not word.isupper() else word for word in words]
+    value = "".join(words)


A side-effect of this is that a leading delimiter gets stripped. So _MyClass and __MyClass both become MyClass. This may not be a problem, but it is a breaking change.

Good catch. I wonder if we should stay consistent or not. What do you think?

After thinking about it a bit, I think it's better to remove the leading underscores since that has the semantics in Python of being private. Nothing in the generated interfaces should be private so... this new behavior seems better.

openapi_python_client/utils.py

…zation.

codecov · 2021-04-25T19:29:51Z

Codecov Report

Merging #375 (000c3d3) into main (83a360f) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #375   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           46        46           
  Lines         1523      1530    +7     
=========================================
+ Hits          1523      1530    +7

Impacted Files	Coverage Δ
openapi_python_client/utils.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 83a360f...000c3d3. Read the comment docs.

dbanty · 2021-04-25T19:33:19Z

end_to_end_tests/golden-record/my_test_api_client/models/__init__.py

@@ -27,5 +27,5 @@
 from .model_with_union_property_inlined_fruit_type0 import ModelWithUnionPropertyInlinedFruitType0
 from .model_with_union_property_inlined_fruit_type1 import ModelWithUnionPropertyInlinedFruitType1
 from .test_inline_objects_json_body import TestInlineObjectsJsonBody
-from .test_inline_objects_response_200 import TestInlineObjectsResponse_200
+from .test_inline_objects_response200 import TestInlineObjectsResponse200


Another issue 😞. I imagine this is why all of the existing string case libraries get it wrong... so many edge cases. I'll add a unit test for this one.

Yep, that's exactly what I was talking about in my previous comment.

dbanty

Alright, this covers all the cases I can think of now. @ramnes will you take a look through the changes when you get a chance and make sure it looks good to you?

ramnes · 2021-04-26T12:17:13Z

I think there are a few problems but I can't dig right now, please don't merge it yet.

ramnes · 2021-04-28T10:30:58Z

Alright, so #397 is the problem I saw while trying this branch in real conditions. I thought it was linked to the changes here and it's actually not, but it makes it tricky for me to smoke test this branch due to the lot of noise it introduces.

dbanty · 2021-04-28T14:38:28Z

Awesome, thanks for checking it out! I'm going to move forward with this one then and we can squash that bug separately.

ramnes · 2021-05-03T16:28:37Z

I think that one of your commits here introduced two problems:

-            id_=id_,
+            id=id,

and

-    s3config: Union[Unset, S3Config] = UNSET
+    s_3_config: Union[Unset, S3Config] = UNSET

dbanty · 2021-05-03T17:28:13Z

id=id was caused by #407 and is intentional.

The s_3_config thing is unfortunate but intentional... I wanted to make numbers be separate words since we often have things like Response200 which should, in my opinion, be in a module called response_200. I think S3 is a weird case and that if we have to pick one then we should opt for numbers be separate words, not part of larger words. Do you disagree?

ramnes · 2021-05-04T19:19:20Z

Maybe it should only be a separate word if it doesn't follow a capital letter?

Don't depend on stringcase (closes openapi-generators#369)

797b76f

Let's reimplement what we need here so that we don't depend on stringcase anymore (see openapi-generators#369 for rationale). I've added a few test cases for stuff I've seen break along the way.

Merge branch 'main' into stringcase-free

26ac759

dbanty requested changes Apr 25, 2021

View reviewed changes

refactor: Fix unit test to match new behavior, optimize word capitali…

6e8166c

…zation.

dbanty linked an issue Apr 25, 2021 that may be closed by this pull request

The stringcase library is buggy and unmaintained #369

Closed

test: Regen golden-record

3aacf5e

dbanty added this to the 0.9.0 milestone Apr 25, 2021

dbanty reviewed Apr 25, 2021

View reviewed changes

fix(cases): treat numbers as words when processing cases

4990254

dbanty approved these changes Apr 25, 2021

View reviewed changes

Merge branch 'main' into stringcase-free

f5fd5e4

dbanty removed this from the 0.9.0 milestone Apr 26, 2021

dbanty added this to the 0.9.0 milestone Apr 28, 2021

Merge branch 'main' into stringcase-free

000c3d3

dbanty enabled auto-merge (squash) April 28, 2021 14:41

dbanty merged commit 85a070a into openapi-generators:main Apr 28, 2021

ramnes mentioned this pull request May 6, 2021

Enhance split words #413

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't depend on stringcase (closes #369) #375

Don't depend on stringcase (closes #369) #375

ramnes commented Apr 1, 2021

ramnes commented Apr 1, 2021 •

edited

Loading

dbanty commented Apr 1, 2021

ramnes commented Apr 1, 2021

ramnes commented Apr 1, 2021

ramnes commented Apr 1, 2021

dbanty left a comment

dbanty Apr 25, 2021

ramnes Apr 25, 2021

dbanty Apr 25, 2021

codecov bot commented Apr 25, 2021 •

edited

Loading

dbanty Apr 25, 2021

ramnes Apr 25, 2021

dbanty left a comment

ramnes commented Apr 26, 2021

ramnes commented Apr 28, 2021

dbanty commented Apr 28, 2021

ramnes commented May 3, 2021

dbanty commented May 3, 2021

ramnes commented May 4, 2021

Don't depend on stringcase (closes #369) #375

Don't depend on stringcase (closes #369) #375

Conversation

ramnes commented Apr 1, 2021

ramnes commented Apr 1, 2021 • edited Loading

dbanty commented Apr 1, 2021

ramnes commented Apr 1, 2021

ramnes commented Apr 1, 2021

ramnes commented Apr 1, 2021

dbanty left a comment

Choose a reason for hiding this comment

dbanty Apr 25, 2021

Choose a reason for hiding this comment

ramnes Apr 25, 2021

Choose a reason for hiding this comment

dbanty Apr 25, 2021

Choose a reason for hiding this comment

codecov bot commented Apr 25, 2021 • edited Loading

Codecov Report

dbanty Apr 25, 2021

Choose a reason for hiding this comment

ramnes Apr 25, 2021

Choose a reason for hiding this comment

dbanty left a comment

Choose a reason for hiding this comment

ramnes commented Apr 26, 2021

ramnes commented Apr 28, 2021

dbanty commented Apr 28, 2021

ramnes commented May 3, 2021

dbanty commented May 3, 2021

ramnes commented May 4, 2021

ramnes commented Apr 1, 2021 •

edited

Loading

codecov bot commented Apr 25, 2021 •

edited

Loading