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

remove sass support #42

Merged
merged 4 commits into from
Mar 2, 2018
Merged

remove sass support #42

merged 4 commits into from
Mar 2, 2018

Conversation

stoeffel
Copy link
Contributor

@stoeffel stoeffel commented Mar 1, 2018

@stoeffel stoeffel added the WIP label Mar 1, 2018
@stoeffel stoeffel self-assigned this Mar 1, 2018
@stoeffel stoeffel force-pushed the remove-sass-support branch from a871c13 to c3299b3 Compare March 1, 2018 00:14
@stoeffel stoeffel removed the WIP label Mar 1, 2018
@stoeffel stoeffel requested a review from cachar March 1, 2018 00:50
Copy link

@cachar cachar left a comment

Choose a reason for hiding this comment

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

I grepped for sass and found it in a few more places-

In Parser/Ast.hs, does SourceType still need to have a Sass value? Then we could get rid of the Ast.Sass case over in DependencyTree.hs

Sass is also still mentioned in a couple of comments- on the readme, and in Parser.Require

let wrapped = wrapModule fnName replacedContent
return $ Just wrapped
else return Nothing
lift $ do
Copy link

Choose a reason for hiding this comment

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

oh nice this is cleaner now 😻

@@ -76,12 +60,9 @@ mockConfig =
[]
("." </> "test" </> "fixtures" </> "concat" </> "sources")
("." </> "test" </> "fixtures" </> "concat" </> "sources")
[]
Copy link

Choose a reason for hiding this comment

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

❔ What did this [] do?

Copy link

Choose a reason for hiding this comment

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

N/m it was the sass load paths in the old Config. I get it!

@stoeffel
Copy link
Contributor Author

stoeffel commented Mar 1, 2018

we should merge #43 first

Copy link

@cachar cachar left a comment

Choose a reason for hiding this comment

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


Looks great!

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.

2 participants