Skip to content
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

Let CLI package use custom Marshmallow Field definitions #125

Merged

Conversation

create-issue-branch[bot]
Copy link
Contributor

@create-issue-branch create-issue-branch bot commented May 6, 2021

closes #124

@Flix6x Flix6x self-assigned this May 6, 2021
@Flix6x Flix6x added this to In progress in Pluggability via automation May 6, 2021
@Flix6x
Copy link
Contributor

@Flix6x Flix6x commented May 6, 2021

By moving our custom DurationField to the data package, I also had to move our custom DurationValidationError and FMValidationError definitions there, to avoid having the data package import from the API package. I hope this is not problematic for you?

If approved, I still need to write a changelog entry (and perhaps a documentation entry, too) about how this enables plugins to use FM marshmallow validators in a plugin's custom CLI function definition.

@Flix6x Flix6x marked this pull request as ready for review May 6, 2021
@Flix6x Flix6x requested a review from nhoening May 6, 2021
Pluggability automation moved this from In progress to Reviewer approved May 6, 2021
Copy link
Contributor

@nhoening nhoening left a comment

I love this! Marshmallow for the win!

Instead of putting the new Mixin in data/models/fields.py, I'd simply add it to data/schemas/utils.py I think.
Given that there is now a schemas directory somewhere in data, it brings up the question if the big schemas (asset, user) should move out from the models dir and into the schemas dir.

@Flix6x Flix6x added this to the 0.5.0 milestone May 6, 2021
@Flix6x
Copy link
Contributor

@Flix6x Flix6x commented May 7, 2021

  • Thanks. I had forgotten to move the mixin there.
  • I also moved the other schemas now.
  • I put the changelog entry under Infrastructure / Support, which felt more right to me than listing it as a new feature. What do you think?

@Flix6x Flix6x requested a review from nhoening May 7, 2021
…kage_use_custom_Marshmallow_Field_definitions
@Flix6x Flix6x merged commit 638d0d1 into main May 11, 2021
2 checks passed
Pluggability automation moved this from Reviewer approved to Done May 11, 2021
@Flix6x Flix6x deleted the issue-124-Let_CLI_package_use_custom_Marshmallow_Field_definitions branch May 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Pluggability
  
Done
Development

Successfully merging this pull request may close these issues.

2 participants