-
Notifications
You must be signed in to change notification settings - Fork 93
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
contrib: added codemeta custom fields. #1187
contrib: added codemeta custom fields. #1187
Conversation
41c3fd2
to
354d092
Compare
70a95be
to
09cd610
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.
Content LGTM! nice job! Questions are more on code placement and responsabilities.
tests/contrib/codemeta/conftest.py
Outdated
): | ||
"""Initializes codemeta custom fields.""" | ||
# Add code meta custom fields | ||
running_app.app.config["RDM_CUSTOM_FIELDS"] = CODE_CUSTOM_FIELDS |
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.
Can we overwrite config using the app_config
fixture?
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.
You mean using the fixture app_config
or moving these config's loading to the fixture itself? If the latter, I'd prefer to load them on demand since that's how the end user will load these custom fields. They must be explicitely loaded when they are to be used.
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.
I meant, on demand. You can overwrite the app_config
and then all gets injected automatically, you can also control the scope, etc. e.g.
invenio-rdm-records/tests/conftest.py
Line 214 in d2cede0
def app_config(app_config): |
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.
I ended up doing something different, sorry. I was having quite some trouble to make it work with app_config
, as it gets picked up by base_app
when the app is initialised.
What I am doing now is simply injecting the configs in the app, initialising the codemeta custom fields and rolling the config back afterwards.
I like your idea better, however I ran into multiple issues (some not even related) and this is how I made it work.
09cd610
to
579313e
Compare
Tests are failing but feel free to discuss, namely this comment |
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.
LGTM! Some naming nits, and a possible fixture removal 🙌
fa27fb3
to
0288108
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.
LGTM! but as I am not expert on custom fields maybe someone else should take a look
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.
Some minor comments on mappings/types, and naming nits (I should start paying more attention in the first reviews 😏 )
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.
Needs rebasing and addressing some minor comments :)
bd1e09d
to
3ded3f8
Compare
3ded3f8
to
8e2382b
Compare
closes #1186