-
Notifications
You must be signed in to change notification settings - Fork 52
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
Add backwards compatibility policy #687
Conversation
✅ Deploy Preview for kaleidoscopic-dango-0cf31d ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Thanks for putting this together.
This needs to live at |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dcmcand Good overall! Left some comments, looking forward to the final version!
See what Pavithra wrote regarding where this file should be placed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @dcmcand - I went through and added some comments and suggestions.
I do not know why, but the API sections feel a lot more solid than the ones about Python code - the latter seems like they get too into the weeds (methods, classes, values) and is thin on the more procedural/implementation side.
I like, for example, NumPy's policy https://numpy.org/neps/nep-0023-backwards-compatibility.html#implementing-deprecations-and-removals
- It makes clear when a deprecation is needed vs being too prescriptive or granular
- It provides clear steps needed to implement a deprecation (add deprecation warning, add to the notes, add to the docs) for example within this PR
Deprecated classes, methods, and functions should have a comment and a warning (if possible) in the code stating why they are deprecated and what to use instead. This will encourage developers not to use them without breaking existing code.....
Perhaps this should be tightened to add a deprecation warning, include the version number of the release where this was deprecated, provide alternatives (if applicable), add to release notes and document accordingly with visible deprecation status. (you get the gist)
-
It Also includes the process on how to go about deprecations - see: https://numpy.org/neps/nep-0023-backwards-compatibility.html#decision-making - this needs to be included. We should have this process to ensure deprecations do not go unnoticed but are intentional instead and approved/agreed on by the core team. We can think of a process like
- Open an RFC issue in the project repository
- Follow a consensus-based decision-making approach (ref: https://producingoss.com/en/producingoss.html#consensus-democracy https://www.seedsforchange.org.uk/shortconsensus)
- If consensus is reached, move to implementation and follow the rest of the guidelines described in this doc
|
||
A breaking change for a module means that any element of the module's public API has a breaking change. | ||
|
||
##### Classes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot put my finger on what it is but from here onwards it feels like this needs some work. Will get back to this tomorrow with a fresh set of eyes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take a look at what I added and see if you think it is better. If you still thinks it needs work, I am happy to add more or rework what is there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks better, let's merge and if needed iterate on this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dcmcand The current version looks great, thanks!
I only have 1 or 2 concrete suggestions, for wording and spelling, the rest of my comments are mostly stylistic nitpicks, which I think will make the source doc easier to read.
I agree with most of Tania's suggestions, too, and clarified where I disagree or have questions.
I'd also prefer we wrap this to 80 chars to match our source code (we use line-length = 88
for Python code), but I don't feel strongly about it if it's too much work.
|
||
#### Removing versions of API endpoints | ||
|
||
It is not recommended to remove versions of API endpoints. Removing API endpoints, or versions of endpoints, breaks backwards compatibility and should only be done under exceptional circumstances such as a security vulnerability. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why a change to "backward" here if we use "backwards" everywhere else?
Co-authored-by: Tania Allard <[email protected]>
Co-authored-by: Tania Allard <[email protected]>
Co-authored-by: Tania Allard <[email protected]>
Co-authored-by: Tania Allard <[email protected]>
Co-authored-by: Tania Allard <[email protected]>
Co-authored-by: Tania Allard <[email protected]>
Co-authored-by: Tania Allard <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dcmcand Thanks for working on this! Unfortunately, I've found some issues caused by a recent commit that wrapped long lines. Also, the lint issues need to be fixed to make the tests pass. Once all these issues are addressed, I'll be happy to accept the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dcmcand I've updated my comments using the "suggestions" feature. PTAL
Co-authored-by: Nikita Karetnikov <[email protected]>
Co-authored-by: Nikita Karetnikov <[email protected]>
Co-authored-by: Nikita Karetnikov <[email protected]>
Co-authored-by: Nikita Karetnikov <[email protected]>
@dcmcand The linter is still failing, will review once the CI is green. GitHub also shows that this branch is out-of-date with the base branch, so I suggest updating. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @dcmcand all looks pretty good now.
I left a couple of nits (formatting) that you can or not include, but otherwise this should be ready for merging
https://example.com/api/v2/user | ||
``` | ||
|
||
Explanation: This route has breaking changes between `v1` and `v2`. Code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explanation: This route has breaking changes between `v1` and `v2`. Code | |
**Explanation**: This route has breaking changes between `v1` and `v2`. Code |
|
||
A breaking change for a module means that any element of the module's public API has a breaking change. | ||
|
||
##### Classes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks better, let's merge and if needed iterate on this
Closes #595
This pull request:
Creates a Backwards Compatibility Policy for the conda-store project as worked out by @nkaretnikov, @costrouc, and myself.
@costrouc and @nkaretnikov , please ensure that I have accurately captured our discussion.
@trallard I welcome any comments. Also, where in the repo would you like this policy to live? I was unclear on that so put it in the root for the meantime.