springload / draftjs_exporter

Convert Draft.js ContentState to HTML
https://www.draftail.org/blog/2018/03/13/rethinking-rich-text-pipelines-with-draft-js
MIT License
83 stars 21 forks source link

Add wheel build option #133

Closed Stormheg closed 4 years ago

Stormheg commented 4 years ago

fixes #132

The primary purpose of this PR is to add an make bdist_wheel command to build a python wheel archive. This task is included in both the make publish and make publish-test commands and does not replace source distributions.

I've added a make clean-dist command to clean the dist folder. This command is ran before publishing to make sure we have a clean dist folder.

Stormheg commented 4 years ago

Thanks for the thoughtful review @thibaudcolas!

This is working as expected for me, I’d rather still only have the single "build" command but I can make that change when merging this.

Refactored into a single build command :+1:

Additionally I see Wagtail has this config for their wheel: https://github.com/wagtail/wagtail/blob/master/setup.cfg#L1-L2. Would we need this here as well?

I did some research on what that python-tag does exactly and found PEP 425. It is used to specify compatibility with python versions/implementations. Generated wheels already have py3 in their filename, indicating compatibility with Python 3 (and no Python 2).

To be honest, I don't think adding a python tag will add any significant value.

Before merging though – an important question from #132 was whether this change would be considered a breaking change or not. We’re still publishing as a source archive so I imagine that reduces the likelihood of issues, but I’d suspect lots of installers will default to the wheel if both options are available. Is that going to cause problems for some people?

I've tried wagtail bakery demo with a wheel version of this library and encountered no issues. But perhaps consider testing yourself and making a test release to PyPi?

Like you mentioned, installers will likely default to wheel versions of this library. I think there is a remote chance that causes issues in specific setups. Lets err on the side of caution, there is no harm done in making a major release :slightly_smiling_face:

thibaudcolas commented 4 years ago

Thank you :)

I don’t think making a test release to PyPI will help much except confirming that it "works on my machine" – as you say it’s more about knowing about the remote chances, whether that’s old versions of pip, or other PyPI clients.

It’s very undesirable for this to be done as a major release if it could be done as a patch one, for a few reasons:

Researching this a bit, it looks like pip just does setup.py bdist_wheel without any further processing: https://pip.pypa.io/en/stable/reference/pip_install/#build-system-interface. I guess the next question is whether the project’s setup.py script would produce the same output regardless of the OS and Python version.

thibaudcolas commented 4 years ago

It looks like Django started publishing wheels in v1.6, but also backported the change to v1.5 and just did so on a patch update:

This seems like as good of a sign as any that this should be appropriate.

thibaudcolas commented 4 years ago

It looks like it might be appropriate for us to actually upload wheels to existing releases – see https://github.com/pypa/packaging-problems/issues/75. I’m still researching this though.