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

.sass-cache doesn't *always* land in options['source'] #6500

Merged
merged 2 commits into from Nov 1, 2017
Merged

.sass-cache doesn't *always* land in options['source'] #6500

merged 2 commits into from Nov 1, 2017

Conversation

@DirtyF DirtyF requested a review from a team October 30, 2017 07:17
Copy link
Member

@Crunch09 Crunch09 left a comment

Choose a reason for hiding this comment

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

👍

@parkr
Copy link
Member

parkr commented Oct 31, 2017

I think it's a bug that we don't write into the site source. I think that bug should be fixed in jekyll-sass-converter: jekyll/jekyll-sass-converter#64.

@envygeeks
Copy link
Contributor Author

TBQF, I would consider what you want a bug, my source is my source, if I change my source to say "source" and it's not the current directory, I would be pretty mad that source is being written to by Jekyll instead of the current directory, because that source directory was created to reduce the amount of clutter that Jekyll likes to create.

This is also part of a larger long-standing issue where Jekyll hasn't created a caching directory that can be customized, and is available to everyone, including plugins.

@pathawks
Copy link
Member

This is also part of a larger long-standing issue where Jekyll hasn't created a caching directory that can be customized

Absolutely, this is ultimately the path forward. For now though, I think it's best that we keep the SASS cache inside the source directory, as that is where we write all other cache files (.jekyll-metadata) and where I as a user would expect the cache to be.

Also, it seems like the SASS generator should be responsible for cleaning its own cache. Maybe we need to add a hook to make that possible?

@envygeeks
Copy link
Contributor Author

we keep the SASS cache inside the source directory

The SASS cache directory isn't and never does land within the source directory so changing that behavior now breaks well established behavior. Behavior I was chastised for just recently. It lands within the current working directory and has since inception, it was just never noticed because people rarely adjust the source directory (apparently?)

As far as leaving the cache to SASS, I don't know that it should be that way. They don't necessarily have supporting functions to do that (I could be wrong, I've not browsed the source all that closely) I think Jekyll should still handle it since it's the one that wants the cache (but that's not true either technically since it's enabled by default.) I've stated my opinion but really I'm neutral on leaving this part of the topic.

@pathawks
Copy link
Member

pathawks commented Nov 1, 2017

Maybe we should focus instead on a centralized cache that is available to all plugins, which Jekyll can destroy during jekyll clean

@mattr-
Copy link
Member

mattr- commented Nov 1, 2017

Maybe we should focus instead on a centralized cache that is available to all plugins, which Jekyll can destroy during jekyll clean

I like this idea.

@ashmaroli
Copy link
Member

never does land within the source directory so changing that behavior now breaks well established behavior.

Can we safely assume that no one (users and plugin developers alike) interacts with a .sass-cache manually or programmatically and hence its location can be silently switched to the intended place at source_dir? Or is that going overboard?

@parkr
Copy link
Member

parkr commented Nov 1, 2017

We probably cannot safely assume that, so let’s accept this PR and move forward with a cache folder. Is there a proposal for that?

@ashmaroli
Copy link
Member

so let’s accept this PR and move forward

👍

@DirtyF
Copy link
Member

DirtyF commented Nov 1, 2017

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit f72e2cc into jekyll:master Nov 1, 2017
jekyllbot added a commit that referenced this pull request Nov 1, 2017
@jekyll jekyll locked and limited conversation to collaborators Jul 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants