Skip to content

feat!: Prefer title attributes when naming Python parameters/properties. #606

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

Closed
wants to merge 2 commits into from

Conversation

dbanty
Copy link
Collaborator

@dbanty dbanty commented May 1, 2022

Closes #602

@jselig-rigetti will you give this a try to ensure it fixes the issue you bumped into? And also give it a review please 😄

@dbanty dbanty force-pushed the 602-use-title-to-override-properties branch from 69301df to d9001ee Compare May 1, 2022 18:16
@dbanty dbanty added ✨ enhancement New feature or improvement 🥚breaking This change breaks compatibility labels May 1, 2022
@dbanty dbanty added this to the 0.12.0 milestone May 1, 2022
@codecov
Copy link

codecov bot commented May 1, 2022

Codecov Report

Merging #606 (35e0261) into main (26e7e0f) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #606   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           49        49           
  Lines         1713      1714    +1     
=========================================
+ Hits          1713      1714    +1     
Impacted Files Coverage Δ
...penapi_python_client/parser/properties/__init__.py 100.00% <100.00%> (ø)
..._python_client/parser/properties/model_property.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 26e7e0f...35e0261. Read the comment docs.

@jselig-rigetti
Copy link
Contributor

Thanks for this! Looks good to me, might be overkill but I can see it being desirable to have an exclude/include configurability for this behavior akin to something like #560 - I can take a look at doing this myself if that makes sense (probably doesn't have to be part of this PR)

I saw a the following error in both of the below situations, which is to be expected:

Warning(s) encountered while generating.
Client was generated, but some pieces may be missing

Titles collide, previously not an error but will be one now.

properties:
  b:
    title: a
  c:
    title: a

Title collides with property key, previously and still an error.

properties:
  _a:
    title: a
  a: {}

@dbanty
Copy link
Collaborator Author

dbanty commented May 4, 2022

I don't see a way to solve for the second case—there is always going to be some risk of collision, I think the best we can do is give as many tools as possible to avoid them (e.g., change/set title). Maybe in the case of the first example, we fall back to using the key for both? So it would generate b and c instead of conflicting.

@dbanty dbanty removed the 🥚breaking This change breaks compatibility label May 8, 2022
@dbanty
Copy link
Collaborator Author

dbanty commented May 8, 2022

@jselig-rigetti thinking about this again, I think the better move is going to be to swap around the behavior. So, instead of always preferring title, we go back to the previous behavior of using the property's key as the name. Then, only in cases on conflicts, we switch to using the title. The good news is that means this is no longer a breaking change—it will just support generating code in more cases. The bad news is that it'll be a lot more work I think 😬

@dbanty
Copy link
Collaborator Author

dbanty commented Jun 3, 2022

Closing in favor of an override for property names via config (@jselig-rigetti is that somewhere you can upstream it?)

@dbanty dbanty closed this Jun 3, 2022
@dbanty dbanty deleted the 602-use-title-to-override-properties branch June 3, 2022 01:45
@dbanty dbanty removed this from the 0.12.0 milestone Aug 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ enhancement New feature or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Client generation error when property names differ only by underscore prefix / suffix
2 participants