-
Notifications
You must be signed in to change notification settings - Fork 671
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
Conversation
0edbcaf
to
3264340
Compare
3264340
to
ca3bd63
Compare
ca3bd63
to
32c990f
Compare
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
What do you think about having an optional |
FYI: These classes seem like ones to fail before this patch.
|
Here is a Proof-of-Concept for this: |
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. :) |
32c990f
to
faa421d
Compare
faa421d
to
8cf5031
Compare
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@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? |
Looks good to me @nejch Thanks. |
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