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

use combineStyleSheets #45

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

use combineStyleSheets #45

wants to merge 1 commit into from

Conversation

gregwebs
Copy link
Member

@gregwebs gregwebs commented Dec 1, 2014

This also shows how to keep static references out of Foundation.hs
That makes upgrading easier

This also shows how to keep static references out of Foundation.hs
That makes upgrading easier
@snoyberg
Copy link
Member

snoyberg commented Dec 1, 2014

I was trying to keep Settings.StaticFiles as just containing that one template Haskell line. Are you opposed to moving the other code into Foundation?

@gregwebs
Copy link
Member Author

gregwebs commented Dec 1, 2014

Yes, we need to move as much code as possible out of Foundation.hs. In general that file tends to grow uncontrollably and can be very difficult to diff when upgrading scaffolding.

However, there are other places it could be placed, such as a new module. But it is a small amount of code for a new module. Why do you want to keep StaticFiles down to 1 TH line?

@snoyberg
Copy link
Member

snoyberg commented Dec 1, 2014

Because I seem to have the exact opposite feeling here as you do: I don't want users to have to dig through multiple files to figure out what's going on. As it stands now, Settings.StaticFiles is essentially a library function that, for technical reasons, has to be part of the scaffolding itself. Spreading code in there now adds it to the modules that a user must read and understand.

@snoyberg
Copy link
Member

snoyberg commented Dec 1, 2014

Also, do people really diff their sites to upgrade scaffolding? That seems strange to me, I don't know if I've ever tried to "upgrade" to a new scaffolding.

@paul-rouse
Copy link

I'll comment since I do "upgrade" scaffolding in existing sites, at least to the extent of tracking the changes and deciding whether I want to apply them. However, it is absolutely painless, so I don't understand how it affects this discussion.

@gregwebs
Copy link
Member Author

gregwebs commented Dec 1, 2014

With respect to diffing: there is a mental diffing you have to do in your head even if the automatic diff is not very useful. But I think the bigger problem this is addressing is that Foundation.hs is a huge file that is difficult to break up and ends up with many odds and ends to satisfy orphans and imports.

With respect to moving across files, if I can quickly grep for something then moving across files is a non-issue. If someone cannot grep for an identifier and find it in a module in their app then they are not ready to use Yesod.

But in fact, in this case moving across files actually makes things easier to understand. Where did css_bootstrap_css come from? Previously if I grepped for it, I would not find it. This is such a fundamental problem that I patched GHC 7.10 to fix this. There are a lot of things that get imported into Foundation.hs. With this change css_bootstrap_css now resides in the module it was created. So when I grep for defaultCss I am conclusively led to StaticFiles.hs and I see css_bootstrap_css in the module that it was created in.

@snoyberg
Copy link
Member

snoyberg commented Dec 1, 2014

That seems like a weak argument to me. We're not really fixing the grep issue in general, we're just maybe getting lucky on this one example. In any event, there's a much more resilient solution to discovering where identifiers come from: using cabal haddock. I'd much rather we just educate users to rely on that more often, as it's a far more general purpose solution to the problem.

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