-
Notifications
You must be signed in to change notification settings - Fork 7
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
Rename IntegrationConfiguration settings property to settings_dict #1303
Rename IntegrationConfiguration settings property to settings_dict #1303
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1303 +/- ##
==========================================
- Coverage 89.85% 89.82% -0.04%
==========================================
Files 210 208 -2
Lines 28655 28549 -106
Branches 6556 6545 -11
==========================================
- Hits 25749 25644 -105
Misses 1893 1893
+ Partials 1013 1012 -1
☔ View full report in Codecov by Sentry. |
380eed6
to
925d28a
Compare
925d28a
to
0f51c60
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just one minor comment, but certainly not a showstopper.
settings = config.settings_dict.copy() | ||
settings[ExternalIntegration.TOKEN_AUTH] = link.href | ||
config.settings = settings | ||
config.settings_dict = settings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: In some cases we're using the variable settings
and in others (e.g., core/model/collection.py
ll. 363-365), settings_dict
. Would be nice to keep these consistent throughout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been hanging out there for awhile, so I'd like to get it merged. I'll try to address this comment in a follow up PR.
Description
Rename the
IntegrationConfiguration
settings
property tosettings_dict
. This aligns properties onIntegrationConfiguration
with those onLibrary
making the code a bit easier to reason about.This also opens up the opportunity to add a settings property on IntegrationConfiguration that loads the settings class from the settings_dict, similar to the method on
Library
.Motivation and Context
While working on PP-94, it was confusing the different properties used to store settings between
IntegrationConfiguration
andLibrary
now that #1281 has gone in.How Has This Been Tested?
Checklist