Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Replace marshmallow-enum with native marshmallow.fields.Enum #227

Merged
merged 3 commits into from
Jan 8, 2023

Conversation

otonnesen
Copy link
Contributor

Addresses #225

@dairiki
Copy link
Collaborator

dairiki commented Jan 7, 2023

The CI test is failing under python 3.6.

Support for python 3.6 was dropped in marshmallow 3.15 (and Enum support was only added in marshmallow 3.18.)

Two options (perhaps there are others) appear to be:

  • Drop python 3.6 support in marshmallow_dataclass. The last time I asked, @lovasoa was against doing that. He'll have to say whether it's time to do that yet.
  • Add extra code/smarts so as to use marshmallow.fields.Enum when it's available, but be able to fall back to using marshmallow_enum.EnumField when it's not.

@mivade
Copy link

mivade commented Jan 8, 2023

Python 3.6 reached its end of life over a year ago now. I think it would make sense to drop support for it.

@otonnesen
Copy link
Contributor Author

In the interest of keeping this diff self-contained, I'll just add the extra code to keep python 3.6 support as you mentioned. I can open another PR to discuss dropping 3.6 support as well if we like.

@dairiki
Copy link
Collaborator

dairiki commented Jan 8, 2023

Python 3.6 reached its end of life over a year ago now. I think it would make sense to drop support for it.

I do not disagree, however, the last time this came up, @lovasoa wanted to maintain 3.6 support, citing the number of weekly downloads. Since then, download numbers have dropped a bit, but not substantially.

image

One option might be to bump our minor version number, dropping support for py 3.6 in the new version, and maintain two branches (an 8.5.x that supports py3.6, and an 8.6.x which does not).

But, as long as it's not too painful to maintain 3.6 support in the current codebase, it's probably cleaner and less work just to do so.

@dairiki
Copy link
Collaborator

dairiki commented Jan 8, 2023

@otonnesen Would it be worthwhile (and actually work) to make the [enum] extra conditional on python version?

E.g., in setup.py:

EXTRAS_REQUIRE = {
    "enum": [
        "marshmallow-enum; python_version<'3.7'",
        "marshmallow>=3.18; python_version>='3.7'",
    ],
    ...
}

@dairiki
Copy link
Collaborator

dairiki commented Jan 8, 2023

@otonnesen Would it be worthwhile (and actually work) to make the [enum] extra conditional on python version?

Never mind. I see you just did that! 😆

@otonnesen
Copy link
Contributor Author

Yeah just tested on 3.6 and 3.10 and it does appear to work as I'd expect

@dairiki
Copy link
Collaborator

dairiki commented Jan 8, 2023

Will wait for further comments for a couple of days before merging, but LGTM. Thank you @otonnesen!

@lovasoa
Copy link
Owner

lovasoa commented Jan 8, 2023

Good for me!

@dairiki dairiki merged commit 614f6a1 into lovasoa:master Jan 8, 2023
@dairiki
Copy link
Collaborator

dairiki commented Jan 9, 2023

Released, just now, in 8.5.11. Thank you, @otonnesen!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants