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

[#2829] add autofix for number-no-trailing-zeros #2947

Merged
merged 1 commit into from
Oct 10, 2017
Merged

[#2829] add autofix for number-no-trailing-zeros #2947

merged 1 commit into from
Oct 10, 2017

Conversation

modosc
Copy link
Contributor

@modosc modosc commented Oct 9, 2017

Which issue, if any, is this issue related to?

#2829

Is there anything in the PR that needs further explanation?

No, it's self explanatory

Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

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

@modosc Thanks! This LGTM. It seems to address the two concerns raised with the number-leader-zero implementation:

  • explicitly checking the node.type.
  • mutating the AST outside of the value-parser loop.

}
};
};

function removeTrailingZeros(input, startIndex, endIndex) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe best place utils?

Copy link
Member

Choose a reason for hiding this comment

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

Is it used by more than rule? If so, then yes I agree. Otherwise, it's probably more helpful to maintainers to keep it here.

@modosc modosc merged commit 445d0bf into stylelint:master Oct 10, 2017
@modosc modosc deleted the number-no-trailing-zeros branch October 10, 2017 15:30
@jeddy3
Copy link
Member

jeddy3 commented Oct 10, 2017

@modosc Thanks again for this.

Please remember to update the CHANGELOG when merging a PR as it's easy for us to loose track.

I did this one for you in 2277cb7

  • Added: number-no-trailing-zeros autofix (#2947).

@modosc
Copy link
Contributor Author

modosc commented Oct 10, 2017

@jeddy3 does the changelog update become part of the pull request or happen after the merge?

@jeddy3
Copy link
Member

jeddy3 commented Oct 10, 2017

does the changelog update become part of the pull request or happen after the merge?

After the merge (as it helps us avoid conflicts). I just realised that the guidelines aren't that clear. I'll tweak them now.

@modosc
Copy link
Contributor Author

modosc commented Oct 10, 2017

After the merge (as it helps us avoid conflicts). I just realised that the guidelines aren't that clear. I'll tweak them now.

got it - it would be cool to use something to do this automatically, maybe https://skywinder.github.io/github-changelog-generator/ ?

@jeddy3
Copy link
Member

jeddy3 commented Oct 10, 2017

got it - it would be cool to use something to do this automatically, maybe https://skywinder.github.io/github-changelog-generator/ ?

We experimented with automating it, but most things ended up being more trouble than they're worth. I think the current process has little overhead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants