-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 TODO@version stylelint rule (local) #946
Conversation
This pull request is being automatically deployed with ZEIT Now (learn more). 🔍 Inspect: https://zeit.co/primer/primer-css/h5exnmi6b |
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.
Is this a good idea, or am I over-thinking things?
I think it's great for fixing things in patches/minors and then not forget about cleaning up later.
Although not sure about removing .selected
for UnderlineNav
just yet, see https://github.com/primer/css/pull/945/files#r334718266.
376ea21
to
23e21a7
Compare
After taking a fresh look at this, I think having a separate script with different error output is unnecessary here, since the |
This adds a new local stylelint rule that looks for comments in the following form:
// TODO@<version>: <message>
...and compares the
<version>
with either the current version inpackage.json
or thePRIMER_VERSION
environment variable (useful for testing!) and, if the comment version is>=
the current version (in semver), fails the test.Rationale
This offers a more ad-hoc and enforceable way of queueing up tasks for a forthcoming release. I've included #945 in this PR as a test case, which you can validate with:
...which outputs:
The idea is that if we merge this into #925, it should fail until we remove the
&.selected
from the selector and the// TODO@14.0.0: remove &.selected
comment.@simurai what do you think? Is this a good idea, or am I over-thinking things?