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

Fix replacing .scss extension by .css #23

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

maelanleborgne
Copy link

No description provided.

Copy link
Member

@bocharsky-bw bocharsky-bw left a comment

Choose a reason for hiding this comment

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

Thanks for proposing this PR! Do you have a use case when the previous solution does not work for you? Could you share, please? I wonder what pushed you to make this fix.

Also, are we sure there won't be a path like "path/to/file.scss?someHashValue123456" in this spot? Otherwise, the new solution will not fit it

@maelanleborgne
Copy link
Author

Thanks for proposing this PR! Do you have a use case when the previous solution does not work for you? Could you share, please? I wonder what pushed you to make this fix.

Also, are we sure there won't be a path like "path/to/file.scss?someHashValue123456" in this spot? Otherwise, the new solution will not fit it

Hi, sorry for the late reply. I made this PR because it was suggested here by @weaverryan .
It may not be a very common use case, but if a path contains .ts (exemple: /project/dir/assets/.ts/myscript.ts) the compile would fail.
This solution would indeed fail to resolve path ending with arguments like you've described, but I couldn't find a way to reach this method by calling an asset with an argument anyway. I might be wrong, but these methods are used when compiling assets, so that happens when exploring the assets dir and iterating on files, so we won't get arguments on these paths.

I should have written a description for the PR, sorry for that 🙏

Copy link
Member

@bocharsky-bw bocharsky-bw left a comment

Choose a reason for hiding this comment

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

OK, thank you for more explanation on it! That sounds good to me for now, if we have problems with the path ending with arguments in the future - we will fix that later too, would be good to have a use case first.

@bocharsky-bw
Copy link
Member

I created a possible fix for the failed PHPStan job here in #26

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.

3 participants