Skip to content

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

Merged
JohnVillalovos merged 2 commits into
masterfrom
fix/parse-cli-objects-camelcase
May 18, 2021
Merged

fix(cli): fix parsing CLI objects to classnames #1290
JohnVillalovos merged 2 commits into
masterfrom
fix/parse-cli-objects-camelcase

Conversation

@nejch

@nejch nejch commented Feb 9, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

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 mentioned this pull request Feb 9, 2021
Closed
@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

codecov-io commented Feb 26, 2021 •
edited
Loading

Copy link
Copy Markdown

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

JohnVillalovos commented Mar 7, 2021

Copy link
Copy Markdown
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

JohnVillalovos commented Mar 7, 2021

Copy link
Copy Markdown
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
JohnVillalovos reviewed Mar 7, 2021
View reviewed changes
Comment thread gitlab/tests/test_cli.py
@JohnVillalovos

JohnVillalovos commented Mar 7, 2021

Copy link
Copy Markdown
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

nejch commented Mar 7, 2021

Copy link
Copy Markdown
Member Author

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

codecov-commenter commented May 15, 2021 •
edited
Loading

Copy link
Copy Markdown

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

nejch commented May 15, 2021

Copy link
Copy Markdown
Member Author

@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 JohnVillalovos mentioned this pull request May 18, 2021
Closed
@JohnVillalovos

JohnVillalovos commented May 18, 2021

Copy link
Copy Markdown
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

Reviewers

@JohnVillalovos JohnVillalovos JohnVillalovos approved these changes
@max-wittig max-wittig Awaiting requested review from max-wittig

Assignees

@JohnVillalovos JohnVillalovos

Labels

None yet

Projects

None yet

Milestone

v3.0.0

Development

Successfully merging this pull request may close these issues.

Gitlab CLI typo in current-user-gpg-key

5 participants

@nejch @codecov-io @JohnVillalovos @codecov-commenter @max-wittig