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

Better releases #593

Merged
merged 14 commits into from May 24, 2017

Conversation

Projects
None yet
3 participants
@mpacer
Copy link
Member

mpacer commented May 24, 2017

In the process of making a release, thought I'd add some better documentation (rather than just following a hodgepodge of notebook and ipython release docs, and also wanted to add explicit checks for the version number to avoid nonvalid version numbers that have caused problems in the past.

@Carreau I figure you'll care the most of everyone about this.

Just want to get this settled and then I'll make the next release. I have these thoughts every time I want to make a release and this time I just wanted to deal with some of the loose ends that are always left dangling before making the actual release.

Also, technically we weren't using a pep440 compliant version number before I don't know why it wasn't causing a problem but hey, no harm in doing so just briefly before releasing.

Assign all merged PRs to milestones
-----------------------------------

Go to GitHub and assign all PRs that have been merged to milestones.

This comment has been minimized.

@takluyver

This comment has been minimized.

@mpacer

mpacer May 24, 2017

Author Member

Great idea.

Create the release
------------------

#. Update the changelog to account for all the PRs assigned to this milestone.

This comment has been minimized.

@takluyver

takluyver May 24, 2017

Member

Should this explicitly mention where the changelog file is?

This comment has been minimized.

@mpacer

mpacer May 24, 2017

Author Member

Relative to the top level? or relative to the location of this file?

git push upstream
git push upstream --tags
#. Push tages on master forgetting to push ``--tags`` too.

This comment has been minimized.

@takluyver

takluyver May 24, 2017

Member

This looks like a confused copy of the point immediately above?

This comment has been minimized.

@mpacer

mpacer May 24, 2017

Author Member

yes i think I was in the middle of splitting one thing in the original notebook, but then realised that I could just have the raw code as two lines.

@@ -0,0 +1,80 @@
.. _nbconvert_release:

Making an ``nbconvert`` release

This comment has been minimized.

@takluyver

takluyver May 24, 2017

Member

Have you checked a built copy of this? I can't remember if backticks in headings work.

This comment has been minimized.

@mpacer

mpacer May 24, 2017

Author Member

Yes, they do work.

@@ -0,0 +1,83 @@
# import pep440 # cannot be imported as described below
import re

This comment has been minimized.

@takluyver

takluyver May 24, 2017

Member

This file looks like it's copied from somewhere - can we have a comment at the top clearly saying where, including what version (or commit, if necessary)? We may also need to include a copyright notice and license text.

This comment has been minimized.

@mpacer

mpacer May 24, 2017

Author Member

It's @Carreau's pep440, github (to which you've committed, which is why I assumed this would be caught), we can add a copyright notice if you like. Basically I'd just use it directly but for the fact that it can't be built with setuptools. That seemed like it was a design choice and if not I was going to make a PR today.

This comment has been minimized.

@mpacer

mpacer May 24, 2017

Author Member

Also I should mention I thought that I had included that comment at the top and in the main text, I guess the only thing that's missing is a github link I will fix that.

This comment has been minimized.

@takluyver

takluyver May 24, 2017

Member

Yeah, put an explicit Github link and a commit ID, for good measure.

I don't personally mind about the copyright notice & license text, and @Carreau may well not mind either. In general, though, if we're including code from another project, we should include those things - it's a typical condition of the licenses which allow you to redistribute that code.

This comment has been minimized.

@Carreau

Carreau May 24, 2017

Contributor

You may also just copy past the is_cannonical from pep 440 itself that was added recently to make this kind of things easier.

I'll make my pep440 easier to vendor.

This comment has been minimized.

@mpacer

mpacer May 24, 2017

Author Member

Got it from pep440 itself Appendix B.

setup.py Outdated
'serve': ['tornado>=4.0'],
'execute': ['jupyter_client>=4.2'],
}
s = []
[s.extend(values) for values in extra_requirements.values()]

This comment has been minimized.

@takluyver

takluyver May 24, 2017

Member

I don't like using list comprehensions for side effects. I'd either do this with a regular for loop:

for reqs in extra_requirements.values():
    s.extend(reqs)

Or get clever with sum:

s = sum(extra_requirements.values(), [])

This comment has been minimized.

@mpacer

mpacer May 24, 2017

Author Member

I did not know that sum could be applied to lists. I'm intrigued because that could allow: extra_requirements['all'] = sum(extra_requirements.values(), []). Which is perhaps a bit too clever but totally does the trick.

This comment has been minimized.

@takluyver

takluyver May 24, 2017

Member

Yep, I think that would be OK.

I believe that sum can accept anything which you can combine with the + operator. If you're not using it for numbers, though, you need to give the second argument for the starting value.

setup.py Outdated
extra_requirements['all'] = list(set(s))

extras_require = setuptools_args['extras_require'] = {
**extra_requirements

This comment has been minimized.

@takluyver

takluyver May 24, 2017

Member

This syntax is new in Python 3.5. If we need a copy of the dictionary, I think extra_requirements.copy() should work. I'm not sure that we do need a copy, though - I think you can just set them to the same object.

@mpacer

This comment has been minimized.

Copy link
Member Author

mpacer commented May 24, 2017

@takluyver do you or @Carreau have any idea about why this makes entrypoints unavailable?

@takluyver

This comment has been minimized.

Copy link

takluyver commented on ab3715e May 24, 2017

Since I know you like to understand things, I took a quick look at what was going on. Having is_canonical outside the other function would normally work. I haven't entirely sussed it, but I'm pretty sure it's a result of calling exec() with two namespaces rather than one. They become a global and a local namespace, but that means that functions you define become closures, which do some weird things. We have some really old issues open in IPython about similar problems.

If you give exec just one namespace, it defines things in globals, which is more predictable - functions keep a reference to the globals dict where they were defined, and use that when looking up variables.

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented May 24, 2017

I don't know what was going on with entrypoints, but it seems to be working now.

setup.py Outdated
'serve': ['tornado>=4.0'],
'execute': ['jupyter_client>=4.2'],
}
extra_requirements['all'] = sum(extra_requirements.values(), [])
extras_require = setuptools_args['extras_require'] = extra_requirements

This comment has been minimized.

@takluyver

takluyver May 24, 2017

Member

I don't think we need to have this called both extra_requirements and extras_require. There's nothing in the code after this using it by either name; its only in setuptools_args that it's used.

This comment has been minimized.

@mpacer

mpacer May 24, 2017

Author Member

Taken care of.

mpacer added some commits May 24, 2017

Comments taken care of.

@Carreau Carreau merged commit 8cda589 into jupyter:master May 24, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mpacer mpacer added this to the 5.2 milestone May 24, 2017

@mpacer mpacer added to_changelog and removed to_changelog labels May 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment