-
Notifications
You must be signed in to change notification settings - Fork 211
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
Added support of @import directives in sass #378
Conversation
13aba6d
to
e60e53c
Compare
SonarQube analysis reported 7 issues Note: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:
|
This feature depend on VirtoCommerce/vc-storefront#378
|
||
public string ToAbsolutePath(string path) | ||
{ | ||
throw new NotImplementedException(); |
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.
If it a part of interface implementation then it must be implemented anyway. Otherwise, this is a violation of SOLID principles
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.
It is 3rd party interface and we can't change it.
throw new NotImplementedException(); | ||
} | ||
|
||
public string ReadFile(string path) |
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.
If you read the file why you return the string?
Maybe ReadFileAsString should be better named in this context
{ | ||
return false; | ||
} | ||
return _contentBlobProvider.PathExists(path); |
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.
cosmetic note: we have a stylecop rule which required add empty space after closed brackets.
see: https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1513.md
https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1508.md
we going to use stylecop in all backend projects so it can be simply corrected now
|
||
namespace VirtoCommerce.LiquidThemeEngine | ||
{ | ||
public interface ISassFileManager: IFileManager |
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.
It would be great to add some comments to this abstraction with a description of the purpose.
|
||
public string ToAbsolutePath(string path) | ||
{ | ||
throw new NotImplementedException(); |
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.
It is 3rd party interface and we can't change it.
About workaround with directories. SASS/SCSS allow to make import like this:
If you have both
mixins
folder andmixins.scss
, compiler will throw an exception because it can't determine which one use.