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

1.2 Cleanup #123

Closed
5 tasks done
Snugug opened this issue Sep 6, 2015 · 12 comments
Closed
5 tasks done

1.2 Cleanup #123

Snugug opened this issue Sep 6, 2015 · 12 comments
Assignees
Milestone

Comments

@Snugug
Copy link
Member

Snugug commented Sep 6, 2015

Go through all of everything and make sure everything is clean and consistent.

  • Options in rules documentation all formatted the same
  • Rules with no options all are in form of no-{{rule}}
  • All rule options are dasherized
  • All rules have documentation
  • Divide tests in to individual files
@Snugug Snugug self-assigned this Sep 6, 2015
@Snugug Snugug added this to the 1.2.0 milestone Sep 6, 2015
@DanPurdy
Copy link
Member

DanPurdy commented Sep 6, 2015

Could this include alphabetically organising main.js so many merge conflicts from that file this time round. 😄

@Snugug
Copy link
Member Author

Snugug commented Sep 6, 2015

I'll do you one better; divide each rule in to its own test

@DanPurdy
Copy link
Member

DanPurdy commented Sep 6, 2015

❤️

I was planning on it!

@Snugug
Copy link
Member Author

Snugug commented Sep 6, 2015

@DanPurdy
Copy link
Member

DanPurdy commented Sep 6, 2015

We've got fixes for #125 and #126 ready to go too. Do you want to roll these into 1.2.0 or get a 1.2.1 out straight after?

@Snugug
Copy link
Member Author

Snugug commented Sep 7, 2015

This rules test rewrite is a surprisingly big job, but on the bright side, I think I can drastically speed up our tests by doing so, in addition to making them easier to maintain

@Snugug
Copy link
Member Author

Snugug commented Sep 7, 2015

@DanPurdy I think we might as well roll them in to 1.2 as it's not released yet

@Snugug
Copy link
Member Author

Snugug commented Sep 7, 2015

@DanPurdy @bgriffith What are your thoughts on renaming color-variable to no-color-literals? The rule, like our other no rules, specifically disallows a single thing from happening and you cannot toggle it on or of, activating it toggles it on and that's it.

@bgriffith
Copy link
Member

If we're only enforcing not using it then I agree with using the no prefix. And since no-color-variable isn't what we're enforcing no-color-literals makes sense to me.

We should also be switching color-keyword to no-color-keyword.

@Snugug
Copy link
Member Author

Snugug commented Sep 7, 2015

I believe I already did color keyword

On Sep 7, 2015, at 9:30 AM, Ben Griffith notifications@github.com wrote:

If we're only enforcing not using it then I agree with using the no prefix. And since no-color-variable isn't what we're enforcing no-color-literals makes sense to me.

We should also be switching color-keyword to no-color-keyword.


Reply to this email directly or view it on GitHub.

@DanPurdy
Copy link
Member

DanPurdy commented Sep 7, 2015

I think it makes a lot of sense yeah.

Have you got any thoughts on #126 @Snugug @bgriffith I'm starting to think the extra option for preventing variables being named the same as colour keywords makes a lot of sense..

In the case you mention above with the no- prefix it would essentially mean a new rule which I actually think is cleaner IF you think this is necessary as a rule at all..

Also I'll get the PR up for the fix anyways once the cleanup is done.

@Snugug
Copy link
Member Author

Snugug commented Sep 7, 2015

Generally speaking I'm a fan of smaller, more specific rules over larger rules with configuration options. Configuration options should be reserved only for when the parsing of the rule as a whole is deterministic on the options present (like indentation and quotes).

Snugug added a commit to Snugug/sass-lint that referenced this issue Sep 7, 2015
@Snugug Snugug mentioned this issue Sep 7, 2015
donabrams pushed a commit to donabrams/sass-lint that referenced this issue Nov 19, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants