Skip to content

fix(cli): fix parsing CLI objects to classnames #1290

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

Merged
merged 2 commits into from
May 18, 2021

Conversation

nejch
Copy link
Member

@nejch nejch commented Feb 9, 2021

Closes #1289. Not sure if relying on requests' internal CaseInsensitiveDict for this is the best idea here but I didn't want to reinvent the wheel. But it's needed since proper camelcase -> kebab-case conversions are not fully reversible (see https://stackoverflow.com/a/1176023)

Fixes the issue for:
CurrentUserGPGKey
LDAPGroup
UserGPGKey

@nejch nejch force-pushed the fix/parse-cli-objects-camelcase branch from 0edbcaf to 3264340 Compare February 15, 2021 23:25
@nejch nejch force-pushed the fix/parse-cli-objects-camelcase branch from 3264340 to ca3bd63 Compare February 22, 2021 23:42
@nejch nejch marked this pull request as ready for review February 22, 2021 23:52
@nejch nejch requested a review from max-wittig February 22, 2021 23:52
@nejch nejch changed the title (proof of concept) fix(cli): fix parsing CLI objects to classnames fix(cli): fix parsing CLI objects to classnames Feb 22, 2021
@nejch nejch force-pushed the fix/parse-cli-objects-camelcase branch from ca3bd63 to 32c990f Compare February 26, 2021 19:00
@codecov-io
Copy link

codecov-io commented Feb 26, 2021

Codecov Report

Merging #1290 (faa421d) into master (48fc907) will increase coverage by 0.02%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1290      +/-   ##
==========================================
+ Coverage   79.92%   79.95%   +0.02%     
==========================================
  Files          73       73              
  Lines        4010     4016       +6     
==========================================
+ Hits         3205     3211       +6     
  Misses        805      805              
Flag Coverage Δ
unit 79.95% <85.71%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
gitlab/v4/cli.py 40.23% <0.00%> (ø)
gitlab/cli.py 55.93% <100.00%> (+2.36%) ⬆️

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 48fc907...faa421d. Read the comment docs.

@JohnVillalovos
Copy link
Member

What do you think about having an optional _arg_name attribute that could be put on classes. If that was present using that as the argument name, otherwise use the what_to_cls function.

@JohnVillalovos
Copy link
Member

FYI: These classes seem like ones to fail before this patch.

CurrentUserGPGKey
current-user-gp-gkey

LDAPGroup
l-da-pgroup

UserGPGKey
user-gp-gkey

@JohnVillalovos
Copy link
Member

What do you think about having an optional _arg_name attribute that could be put on classes. If that was present using that as the argument name, otherwise use the what_to_cls function.

Here is a Proof-of-Concept for this:
#1362

@nejch
Copy link
Member Author

nejch commented Mar 7, 2021

What do you think about having an optional _arg_name attribute that could be put on classes. If that was present using that as the argument name, otherwise use the what_to_cls function.

I actually did this whole namespace lookup dance so that we wouldn't need to manually maintain another attribute in case people forget about it when adding new features 😅 but maybe your approach is clearer. I haven't looked at whether the current approach is problematic for typing. Either way I'll rebase and add some more tests. :)

@nejch nejch force-pushed the fix/parse-cli-objects-camelcase branch from 32c990f to faa421d Compare March 7, 2021 11:04
@nejch nejch added this to the v3.0.0 milestone Apr 27, 2021
@nejch nejch force-pushed the fix/parse-cli-objects-camelcase branch from faa421d to 8cf5031 Compare May 15, 2021 10:16
@codecov-commenter
Copy link

codecov-commenter commented May 15, 2021

Codecov Report

Merging #1290 (8cf5031) into master (f35c73e) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1290      +/-   ##
==========================================
+ Coverage   90.82%   90.83%   +0.01%     
==========================================
  Files          73       73              
  Lines        4020     4027       +7     
==========================================
+ Hits         3651     3658       +7     
  Misses        369      369              
Flag Coverage Δ
cli_func_v4 80.35% <100.00%> (+0.03%) ⬆️
py_func_v4 79.43% <50.00%> (-0.07%) ⬇️
unit 81.89% <87.50%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
gitlab/cli.py 86.08% <100.00%> (+0.90%) ⬆️
gitlab/v4/cli.py 81.32% <100.00%> (ø)

@nejch
Copy link
Member Author

nejch commented May 15, 2021

@JohnVillalovos I just rebased, could you take one last look and merge if it's ok? :)

Based on some other discussions on (not) manually maintaining too many object attributes I think #1362 can be closed if this is merged, WDYT?

@nejch nejch assigned JohnVillalovos and unassigned max-wittig May 15, 2021
@JohnVillalovos
Copy link
Member

Looks good to me @nejch Thanks.

@JohnVillalovos JohnVillalovos merged commit 1508eb7 into master May 18, 2021
@nejch nejch deleted the fix/parse-cli-objects-camelcase branch May 18, 2021 04:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gitlab CLI typo in current-user-gpg-key
5 participants