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

Remove label kwarg #644

Merged
merged 4 commits into from Nov 23, 2015

Conversation

Projects
None yet
2 participants
@patricksnape
Contributor

patricksnape commented Nov 17, 2015

Sorts out #614

patricksnape added some commits Nov 17, 2015

Sneak in include_mapping->return_mapping
include_mapping is a terrible name for returning the mapping
dictionary. So change it to return_mapping
@patricksnape

This comment has been minimized.

Contributor

patricksnape commented Nov 23, 2015

@jabooth is this cool?

@jabooth

This comment has been minimized.

Member

jabooth commented Nov 23, 2015

+1 from me. Just a reminder that when it comes to writing the changelog for 0.6 we should try and document breaking changes, obviously dropping label is one, but include_mapping -> return_mapping should be documented as well.

jabooth added a commit that referenced this pull request Nov 23, 2015

@jabooth jabooth merged commit ac77a75 into menpo:master Nov 23, 2015

4 checks passed

OS X MenpoBot Jenkins build passed No test results found.
Details
clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jabooth jabooth removed the in progress label Nov 23, 2015

@patricksnape

This comment has been minimized.

Contributor

patricksnape commented Nov 23, 2015

The include_mapping -> return_mapping isn't a breaking change because I think that this is totally new unreleased behaviour anyway.

@jabooth

This comment has been minimized.

Member

jabooth commented Nov 23, 2015

@patricksnape cool my bad. (Side note, maybe a CHANGELOG.md file would be useful? As PRs come in if you do a breaking change as part of the the PR you add a line to the file? Can then be used for release docs etc?)

@patricksnape

This comment has been minimized.

Contributor

patricksnape commented Nov 23, 2015

The release notes are already a changelog. I think maybe we should add BREAKING or something to PRs that break something? Then it is easy to grep for it?

@jabooth

This comment has been minimized.

Member

jabooth commented Nov 23, 2015

That's true, could just be a breaking GitHub tag that we start using?

Only reason I suggested a physical file is that at some point someone has to sit down and write a short English description of a breaking change and point to the PR. Doing that job at PR-time seems easier than at release-time? Also, there are sometimes multiple breaking changes per PR, and if possible I think it would be better to document them individually for the release notes.

Maybe a better system than files would just be a gentlemans argreement that all breaking change PRs:

  1. Get the aforementioned breaking tag
  2. Get a block at the top of the PR enumerating clearly the breaking API changes.

That way at release like you said it's easy to filter for breaking changes and just grab the descriptions.

@jabooth jabooth referenced this pull request Nov 23, 2015

Closed

Remove ``label`` kwarg #614

@patricksnape patricksnape deleted the patricksnape:remove_label branch Feb 1, 2016

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