-
Notifications
You must be signed in to change notification settings - Fork 24
docs: add docs for deprecating a feature #74
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #74 +/- ##
=======================================
Coverage 63.18% 63.18%
=======================================
Files 210 210
Lines 22187 22187
=======================================
Hits 14018 14018
Misses 7082 7082
Partials 1087 1087 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
zimeg
left a comment
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.
@mwbrooks Thank you for putting these steps into writing and matching what's now noted in code!
A process around deprecations is so nice to allow useful changes that might not be backwards compatible, and I am glad we can be encouraging in these updates to latest 🎁
One comment I left adds a word but the meanings are so good.
|
|
||
| - Public functionality should be deprecated on the next `semver:major` version | ||
| - Add the comment `// DEPRECATED(semver:major): Description about the deprecation and migration path` | ||
| - Print a warning `PrintWarning("DEPRECATED: Description about the deprecation and migration path")` |
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 will be so helpful in moving to the next major versions! I am a big fan of this pattern, even if it causes noise after an update 📚 ✨
|
|
||
| // Fallback check for slack.json and .slack/slack.json file | ||
| // TODO(semver:major): remove both fallbacks next major release | ||
| // DEPRECATED(semver:major): remove both fallbacks next major release |
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 is so much more obvious! 🙏
I find searches for TODO raised false positives and I'm so glad to align on the go conventions you note 📝
Also, thanks for finding these instances in 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.
I agree, so far this makes assessing the effort involved in removing a deprecation a lot easier!
Co-authored-by: Eden Zimbelman <[email protected]>
|
@zimeg Thanks for catching my grammar mistake! 📝 🐝 Looks like we can go ahead and merge this one! |
Summary
This pull request is a follow-up from PR #70 that adds documentation around deprecating a feature in code:
MAINTAINERS_GUIDE.mdexplaining how to deprecate a public and internal feature// TODO(semver:major):to// DEPRECATED(semver:major):We're aligning on
// DEPRECATED:and// DEPRECATED(semver:major):because Golang appears to use a similar convention. If anyone thinks we should use a different convention, happy to hear it! 👂🏻Requirements