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

feature(auto-fix): Implementation of --fix flag #1089

Merged
merged 6 commits into from
Mar 2, 2019

Conversation

srowhani
Copy link
Member

@srowhani srowhani commented Jun 15, 2017

What do the changes you have made achieve?

  • Base implementation of --fix
  • Tests
    • Maintain coverage %
    • Test fix's already implemented as demonstration of PR capabilities
      • border-zero
      • no-css-comments
      • property-sort-order

Are there any new warning messages?

No, fix will only run if given fix config option.
If fix is run, the only difference is it will rebuild the parsed ast correctly, and write the new one into that file.

Have you written tests?

Partially, yes. Currently in the process of writing more tests.

Have you included relevant documentation

Yes, have added some explanation on how to run fix, and how to add your own rule fixes.

Which issues does this resolve?
#904
<DCO 1.1 Signed-off-by: SEENA ROWHANI seenarowhani95@gmail.com>

Let me know of any concerns or questions. I understand this PR can be risky from your perspective, so I'll do my best to address any comments.

Also, side-note, help adding rules / testing them would be really helpful. So if anyone wants to contribute, feel free to PR my fork 🔥

Closing #997 in favour of this.

@coveralls
Copy link

coveralls commented Jun 15, 2017

Coverage Status

Coverage increased (+1.1%) to 96.164% when pulling c230b34 on srowhani:feature/auto-fix into d96ed96 on sasstools:feature/auto-fix.

@gmccue
Copy link

gmccue commented Aug 8, 2017

@DanPurdy @danieldafoe any chance of a merge of this PR sometime soon?

@rodolfoprr
Copy link

@srowhani thanks for your PR, it helped me a lot on a project where I needed to fix some warnings regarding the property-sort-order rule.

I just had a problem where, in a mixin, I have the following code:

@mixin global-navigation-box-shadow {
  $shadow-blur-radius: spacing(.325);
  $shadow-offset-x: 0;
  $shadow-offset-y: spacing(.5);

  $shadow-spread-radius: spacing(.25);
  box-shadow: inset $shadow-offset-x (-$shadow-offset-y) $shadow-blur-radius (-$shadow-spread-radius) rgba(color('shades-1'), .1);
}

after running --fix:

@mixin global-navigation-box-shadow {
  box-shadow: inset $shadow-offset-x (-$shadow-offset-y) $shadow-blur-radius (-$shadow-spread-radius) rgba(color('shades-1'), .1);
  $shadow-blur-radius: spacing(.325);
  $shadow-offset-x: 0;
  $shadow-offset-y: spacing(.5);

  $shadow-spread-radius: spacing(.25);
}

and running node-sass my-file.scss:

{
  "status": 1,
  "file": "my-file.scss",
  "line": 5,
  "column": 21,
  "message": "Undefined variable: \"$shadow-offset-x\".",
  "formatted": "Error: Undefined variable: \"$shadow-offset-x\".\n        on line 5 of my-file.scss\n>>   box-shadow: inset $shadow-offset-x (-$shadow-offset-y) $shadow-blur-radius (\n   --------------------^\n"

@srowhani
Copy link
Member Author

@rodolfoprr I'll experiment with your snippet, thanks!

@stephanebruckert
Copy link

Good job @srowhani ! Will integrate SASS linting in my app as soon as your PR is merged

@jesperronn
Copy link

This branch has been ready since June 15th, could one of the maintainers please comment or merge this?

I know maintaining open-source projects is hard work, and rarely paid as well. And I fully acknowledge the maintainers lack of abundant free time.

But if you have time just to comment or evaluate this, it would help the project. Thanks a lot!

@DanPurdy
Copy link
Member

DanPurdy commented Nov 6, 2017

@jesperronn It's not ready though? Under the have you written tests section it says partially, in the process of writing more and there are still tick boxes that aren't ticked and a reported issue with no further commits so it's assumed to not be ready. If we hear a little more about it then I'm happy to have a look and go through it and for others to get involved and work on / test this.

I don't mean to be picky with this but it's a big new feature that we haven't had time to do ourselves and we're grateful that @srowhani has taken the time to work on it but we're unsure of the status currently.

@srowhani
Copy link
Member Author

@DanPurdy I've updated the description to accurately describe the PR's state. Unfortunately, cecause of other responsibilities I can't put any more time into this. Since this PR is going into a separate branch, you could pull it in and push further changes regarding this feature directly into the feature/auto-fix branch on sass-lint.

@DanPurdy
Copy link
Member

Thanks @srowhani I'll have a look when i get a chance as im in a similar situation with other responsibilities at the moment. Thanks for your time and effort here.

@awaisabir
Copy link

I need this!

@chinmaymahajan
Copy link

@DanPurdy I'm eagerly waiting to see the auto-resolve option, Will greatly appreciate if you give any update on this. Thanks and Great work!!

@RaulMo7-zz
Copy link

Please give any update.

@yannbf
Copy link

yannbf commented Mar 18, 2018

It's been quite some time.. any updates on this?

@BenoitFroment
Copy link

Impatiently waiting for this

@srowhani
Copy link
Member Author

I plan on publishing a separate addon to implement these features, that will sit on top of the current implementation of sass-lint. For anyone interested, I can post updates here after some progress.

@srowhani
Copy link
Member Author

srowhani commented Apr 9, 2018

I made a first pass at this add-on. You can try it out, if you want, here.

@zewa666
Copy link

zewa666 commented Aug 23, 2018

Hey @srowhani, whats the status of this PR? Would be great to see this as part of sass-lint so that e.g the VSCode extension could start to pick up this feature as well

@srowhani
Copy link
Member Author

srowhani commented Aug 24, 2018

Hello! I think as it stands the sass-lint maintainers are leaving the repo as it is. An IDE extension would definitely be nice to have!

@Johanneke
Copy link

@DanPurdy Has there been any progress on developing this feature?

@jahilldev
Copy link

@srowhani @DanPurdy Any ideas on when we could see this released? Would save a lot of time! Thanks 👍

@srowhani
Copy link
Member Author

srowhani commented Jan 8, 2019

Sorry to keep everyone waiting on this, but due to other responsibilities I don't have as much time as I'd want to be able to commit to this feature actively. I do want to let you know that this is currently available here while actively in development.

The plan is to arrange with the other maintainers once the auto-fix feature is in a stable state, and
have it merged and published with sass-lint.

@srowhani srowhani merged commit fa582f4 into sasstools:feature/auto-fix Mar 2, 2019
@srowhani
Copy link
Member Author

srowhani commented Mar 2, 2019

Merging this for now - though it is stale. No release will be generated from this. The more up to date version can currently be found here, and I'll be spending some time to see about merging and releasing a new version soon. 👍

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

Successfully merging this pull request may close these issues.

None yet