Skip to content

Add validation that the version has been increased compared to master#275

Merged
dekatzenel merged 1 commit intomasterfrom
feature/mandatory-version-bumps
Apr 3, 2020
Merged

Add validation that the version has been increased compared to master#275
dekatzenel merged 1 commit intomasterfrom
feature/mandatory-version-bumps

Conversation

@dekatzenel
Copy link
Copy Markdown
Contributor

@dekatzenel dekatzenel commented Apr 2, 2020

Citrine Python PR

Description

Add automated validation that the version in setup.py has been bumped

PR Type:

  • Breaking change (fix or feature that would cause existing functionality to change)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Adherence to team decisions

  • I have added tests for 100% coverage
  • I have written Numpy-style docstrings for every method and class.
  • I have communicated the downstream consequences of the PR to others.

@dekatzenel
Copy link
Copy Markdown
Contributor Author

Success - the correct failure this time!

Add bumping version to PR template

Bump version (also reflecting prior changes)
@dekatzenel dekatzenel force-pushed the feature/mandatory-version-bumps branch from 99bcdaa to ec15e32 Compare April 3, 2020 15:14
@dekatzenel dekatzenel requested review from maxhutch and mcgalcode April 3, 2020 15:18
@dekatzenel
Copy link
Copy Markdown
Contributor Author

@maxhutch @mcgalcode In addition to general correctness, I would appreciate a sanity check/improvement recommendations on my bash script. If you do a review, please let me know if this is something you feel confident in doing

Copy link
Copy Markdown
Contributor

@maxhutch maxhutch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. One nit on the version bump. I did review but am not super confident in the bash logic.

Comment thread setup.py

setup(name='citrine',
version='0.9.1',
version='0.10.0',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this a patch bump?

Copy link
Copy Markdown
Contributor Author

@dekatzenel dekatzenel Apr 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm including all the things I forgot, i.e. the commits I forgot to do bumps for are minor version bumps

exit 4
fi
fi
fi
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gave this bash script a cursory glance and everything here is good shell scripting:

  • set -eu 👍 I normally add in -o pipefail but there's no piping here so it's N/A
  • quoted arguments to test builtin
  • straightforward control flow

@dekatzenel dekatzenel merged commit a254785 into master Apr 3, 2020
@dekatzenel dekatzenel deleted the feature/mandatory-version-bumps branch April 3, 2020 21:36
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.

3 participants