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 #75: detect sass binary in the path and use it if available #76

Merged
merged 1 commit into from
Oct 11, 2024
Merged

fix #75: detect sass binary in the path and use it if available #76

merged 1 commit into from
Oct 11, 2024

Conversation

drupol
Copy link
Contributor

@drupol drupol commented Oct 9, 2024

This PR fix #75 by doing:

  • Use the binary sass if it is available in the path
  • If the path is not available in the path, it will try to download it

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.

Hm, correct me if I'm wrong but you're trying to prioritize the binary from paths among other available binaries. I see this bundle allows to specify the path to the binary you want to use, see: https://symfony.com/bundles/SassBundle/current/index.html#using-a-different-binary - so it already works the way you need, i.e. you can just specify the path to the local binary from path and it will work for you, right? The only problem with this behaviour is that you need to specify that path manually. In case we want to automate it - we need to follow the next logic IMO:

  1. Check if the binary path is set in the config, i.e. symfonycasts_sass.binary option. if it's set - just use it, no need to find anything because it's explicitly configured
  2. If no symfonycasts_sass.binary option is set - try to find the installed binary from the path as you suggest. If found - use it.
  3. If no installed binary in the path - try to download the latest binary.

@drupol
Copy link
Contributor Author

drupol commented Oct 10, 2024

OK will adjust my PR then ! Thank you for the review!

@bocharsky-bw
Copy link
Member

Also, tests and code styles were fixed in #73 , please, release your PR

@drupol
Copy link
Contributor Author

drupol commented Oct 10, 2024

@bocharsky-bw Ready for another review.

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.

This looks good to me now, thanks for this idea!

@bocharsky-bw bocharsky-bw merged commit 20b9358 into SymfonyCasts:main Oct 11, 2024
12 checks passed
@drupol drupol deleted the push-ynpymuursuln branch October 11, 2024 11:54
bocharsky-bw added a commit that referenced this pull request Oct 11, 2024
Add more information about the bundle's behaviour after #76
@bocharsky-bw bocharsky-bw mentioned this pull request Oct 11, 2024
bocharsky-bw added a commit that referenced this pull request Oct 11, 2024
Add more information about the bundle's behaviour after #76
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.

Proposal to improve SASS binary handling
2 participants