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

Drop requirement for setuptools package from setup.py #211

Merged
merged 1 commit into from
Jan 25, 2017
Merged

Drop requirement for setuptools package from setup.py #211

merged 1 commit into from
Jan 25, 2017

Conversation

jdufresne
Copy link
Contributor

setuptools is shipped with Python, no need to include it as a requirement. It
is bad form to include it as a requirement as pip will try to pull in the
latest version.

@untitaker
Copy link
Contributor

Yeah that wouldn't work anyway since we already import setuptools at the top of setup.py.

I'd like to hear somebody else's opinion on this though.

@untitaker
Copy link
Contributor

In any case this is probably worth a changelog entry.

setuptools is shipped with Python, no need to include it as a requirement.

Further, it is not used by the icalendar package, but only by setup.py.

setuptools is imported by setup.py _before_ dependencies have been installed.
@jdufresne
Copy link
Contributor Author

I have added a note to the changelog. Please let me know if you'd like any wording adjusted. All feedback welcome, thanks.

@kevin-brown
Copy link

I'm 👍 on this change, for the reasons mentioned in the intro post.

Yeah that wouldn't work anyway since we already import setuptools at the top of setup.py.

This applies for cases where setup.py is executed during installation, which is all of the cases for icalendar.

@untitaker untitaker merged commit 4ff6452 into collective:master Jan 25, 2017
@untitaker
Copy link
Contributor

Ok, thanks!

@jdufresne jdufresne deleted the no-require-setuptools branch January 25, 2017 19:41
@mauritsvanrees
Copy link
Member

I agree with the change, but I think there are a few mistakes in the reasoning here, so let me add some comments.

  • setuptools is not shipped with Python. Maybe in some versions, but certainly not all.
  • Just because setuptools is imported in the top of the file does not mean that adding it in install_requires is useless: as far as I know, packaging tools can actually handle this.
  • But setuptools is usually only needed when you have a namespace package. That is not the case here, so removing setuptools is fine.

BTW, I wouldn't call this a 'breaking change'. See this question and answer on semver.org that says an update in dependencies does not mean you are suddenly backwards incompatible. There will be cases where it is better to do that, but not here.
So I have removed that sub header and filed the changelog entry under 'Bug fixes'.

I have released version 3.11.3 with this change. Thank you!

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

Successfully merging this pull request may close these issues.

5 participants