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

[doc] Update CONTRIBUTING about PR and versions #731

Merged
merged 1 commit into from
Jan 19, 2021
Merged

Conversation

woshilapin
Copy link
Contributor

@woshilapin woshilapin commented Jan 18, 2021

Some help about why and how we update versions of Cargo.toml files in the Pull Request.

CONTRIBUTING.md Outdated

### Update version

The PR owner is the best to understand which evolution and breaking
Copy link
Contributor

Choose a reason for hiding this comment

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

it feels a bit awkward to bump the version if the previous has not been released no?
I’d understand it if this was continuously released but it does not seems the case.
Isn’t it better to do it at release time ?

Copy link
Contributor Author

@woshilapin woshilapin Jan 18, 2021

Choose a reason for hiding this comment

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

it feels a bit awkward to bump the version if the previous has not been released no?

Yes, and maybe it's not clear that this is exactly what I'm trying to explain here, but you should not bump the version if it's already alright.

Examples:

  1. Version 1.0.0 is already released and Cargo.toml contains 1.0.0
  2. Alice makes a bug fix, she bumps the Cargo.toml to 1.0.1
  3. Bob makes another bug fix; he sees 1.0.1 is not released yet, so he doesn't need to make a change
  4. Carl makes a breaking change, he bumps to 2.0.0
  5. Donald makes a new feature; he sees there is already a major bump, so it does not need to bump the version.

And notice how at each step, we could have published a release right away.

Isn’t it better to do it at release time ?

The pain to do that at release time is that the owner of the release doesn't necessarily know the details of each PR (is it a breaking change or not?), and would need to either do a new PR only to bump a version (that's a shame for just a version bump) or push a new commit directly to master (not a fan of pushing commits directly to master and actually, master might just be protected against push).

Copy link
Contributor

Choose a reason for hiding this comment

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

hum isn’t it difficult for Bob to know that there has not been a release?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, instruct to compare with crates.io is the clearest you can do here, I'd add it to the sentence :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've introduced it as a link, tell me if that's fine.

CONTRIBUTING.md Outdated

### Update version

The PR owner is the best to understand which evolution and breaking
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, instruct to compare with crates.io is the clearest you can do here, I'd add it to the sentence :)

pbougue
pbougue previously approved these changes Jan 18, 2021
Copy link
Member

@datanel datanel left a comment

Choose a reason for hiding this comment

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

I think updating the version should not be something mandatory for an external contributor (not from the team). he should not have to think about it systematically but the team does.

@woshilapin
Copy link
Contributor Author

I think updating the version should not be something mandatory for an external contributor (not from the team). he should not have to think about it systematically but the team does.

We might mention it if the contributor forget, but I would like to avoid as much as possible PR exclusively for bumping. Said differently, I'm totally fine if an external contributor forget to do it (we too forget about it sometimes), but yet, I'd like it to be documented, and we will remind to the contributor to do it before merging the PR.

@datanel
Copy link
Member

datanel commented Jan 19, 2021

My point is we must not force the developer to always change the version if needed. If he wants to bump the version it's ok but it's the responsability of the team to think about if when releasing.

@woshilapin
Copy link
Contributor Author

My point is we must not force the developer to always change the version if needed. If he wants to bump the version it's ok but it's the responsability of the team to think about if when releasing.

I'd rather document our process, and be OK with an external contributor forgetting about it, than not documenting our processes.

@woshilapin
Copy link
Contributor Author

My point is we must not force the developer to always change the version if needed. If he wants to bump the version it's ok but it's the responsability of the team to think about if when releasing.

I've added commit a42eca5 to smooth the tone a little. The idea being that we might even propose a PR on top of the contributor's branch to help him do it. Or help him decide which version's bump should be done. In general, we do not have a lot of external contributors, so I'd say we will be extremely careful to please them and help them as much as possible.

@woshilapin woshilapin merged commit b00d935 into master Jan 19, 2021
@woshilapin woshilapin deleted the doc-pr branch January 19, 2021 12:06
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.

4 participants