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

Proposal for new project skeleton #75

Open
wants to merge 5 commits into
base: postgres
Choose a base branch
from

Conversation

s9gf4ult
Copy link

This is related with 997 issue

@gregwebs
Copy link
Member

I don't know what value the App. prefix adds: it seems like it could just be removed.
A module space Route is added, but the meaning seems no different from Handler.
Also, the name change of the Import modules does not seem to increase organization.
Having an Import/ folder is nice because it suggests a place to deal with modifying imports of external packages.

@s9gf4ult
Copy link
Author

The meaning of Route is different: while Handler contains only handler modules Route contains everything related to specific route. The handlers of route are placed in module App.Route.RoutePath.Handler so the modules are grouped by route first and by functionality second.

Looks like App could be cutted, byt it might increase inconsistency if you decide to place in your project some code which is not related to your Yesod application directly, servant subsite for example (which should be placed in other package but anyway)

Imports can be placed in Import.Foundation and Import.NoFoundation instead of App.FoundationImport and App.NoFoundationImport respectively. I beleive it is little strange to call import with Foundation just Import but import without Foundation to call Import.NoFoundation.

@gregwebs
Copy link
Member

gregwebs commented Jun 4, 2015

I beleive it is little strange to call import with Foundation just Import but import without Foundation to call Import.NoFoundation.

yes, I agree. It is only that way just for backwards compatibility, but we can change it to Import.Foundation and Import.NoFoundation.

@gregwebs
Copy link
Member

gregwebs commented Jun 4, 2015

Looks like App could be cutted, byt it might increase inconsistency if you decide to place in your project some code which is not related to your Yesod application directly, servant subsite for example (which should be placed in other package but anyway)

Do you mean increase the change for module name clashes? In that case it is always possible for the servant subsite to take the same approach and use an App prefix. A prefix to prevent collision should not be generic, so better would be to use the site name. But then it is probably better just to use package qualified imports as necessary.

@gregwebs
Copy link
Member

gregwebs commented Jun 4, 2015

The meaning of Route is different: while Handler contains only handler modules Route contains everything related to specific route. The handlers of route are placed in module App.Route.RoutePath.Handler so the modules are grouped by route first and by functionality second.

This makes sense. The way I broke things out previously was to have Handler.Home import Handler.Home.Form. That way the simple case only has 1 organization folder to go though (Handler), whereas after this pull request there are always 2 folders to go through to get to the Handler (Route.Path).

@gregwebs
Copy link
Member

gregwebs commented Jun 4, 2015

I like the DB module prefix.

@s9gf4ult
Copy link
Author

s9gf4ult commented Jun 4, 2015

Ok, I agree with squashing App prefix.

If I understand you agree with Route prefix (or not?)

And what do you thing about module App.Handler (which will just Handler, or maybe Handlers after squashing App)? This module just reexports all handlers to import in Application. There is cases when yesod scaffolding fails to insert import Handler.NewHandler into Application module. I think that putting all this long imports list to one App.Handler module will simplify scaffolding as well.

@gregwebs
Copy link
Member

gregwebs commented Jun 4, 2015

I like having a specific module for re-exporting the handlers.

The main problem with changing the Handler re-export is what you alluded to: that yesod-bin already has a command to add a handler (which I have never used) which adds an import assuming placing the import into Application. The command will need to maintain compatibility with the previous scaffold.

I prefer to to not use plural (Handlers) module names and just stick with the singular (Handler).
Other possible names would be Handler.Import or Handler.Application, but now that we have src/ it is probably fine to put more things at the top level of src/.

@s9gf4ult
Copy link
Author

s9gf4ult commented Jun 4, 2015

Maybe even Import.Application to just put all the importable modules in one place?

The maintaining looks simple: if we did not find module Import.Application then use old way and put import to Application.

@gregwebs
Copy link
Member

gregwebs commented Jun 5, 2015

Import. is generally used to organize external package imports (and possible to add some functionality on top of them), but this is organizing the internal application modules. For that reason I favor just using Handler.

@gregwebs
Copy link
Member

gregwebs commented Jun 5, 2015

I cannot say that I favor the Route prefix approach over nesting things under Handler. Again, the reason is that if Handler.Home imports Handler.Home.Form, then Handler.Home has only 1 folder to get to the handler. With Route.Home.Handler then there are 2 folders to get to the handler. Also, Handler.Home.Form is compatible with the current scaffolding.

@dpwiz
Copy link

dpwiz commented Jun 5, 2015

The question is not about naming this module App or Route or whatever. It's more like an example of site section prefix and can be Blog.*, Payment.*, API.* or even Site.* (for site-wide things). Having this right from starts sets a good example for code organization. Because lots of sites tend to ignore grouping and after a couple of iterations end up with either really huge modules or lots of 2-function modules. A little bit of structure prevents early degradation and keeps the maintenance costs down.

@s9gf4ult
Copy link
Author

s9gf4ult commented Jun 5, 2015

I agree with @wiz. Putting all this Foundation and Application under one prefix like e.g. Site will also reduce code mess when there is subsites. Consider:

DB.Model
Handler.Home
....
Subsite1.DB.Model 
Subsite1.Handler.Ping

versus

Site.DB.Model
Site.Handler.Home
....
Subsite1.DB.Model 
Subsite1.Handler.Ping

It is about code organisaion.

@gregwebs
Copy link
Member

gregwebs commented Jun 5, 2015

I am open to the site prefix idea. I just don't agree that a generic App or Site adds value: it should be specific.

@dpwiz
Copy link

dpwiz commented Jun 5, 2015

Specific... like what?

Perhaps a better idea would be to skip creating the "HomeR" route/handler and add a yesod start-app <section prefix> command instead (of course this goes straight out of the scope of this PR).

@gregwebs
Copy link
Member

gregwebs commented Jun 5, 2015

Specific... like what?

If you are making a blog app, then Blog. Actually, I am not sure why that would be immediately useful either. My thinking was more that it might be useful if you want to turn the functionality into a subsite.

@s9gf4ult
Copy link
Author

s9gf4ult commented Jun 5, 2015

I think we must stop on some generic prefix, this is the naming issue and user can decide to rename modules later if necessary. Anyway the name of Foundation module or Application are also choosen by default for user and it seems not a problem at all.

@gregwebs
Copy link
Member

gregwebs commented Jun 6, 2015

Foundation and Application actually have meaning though unlike a generic prefix. This does bring up a good question though: shouldn't the prefix also apply to Foundation and Application and everything else?

It seems like we disagree. The best way forward would be to ask for opinions on the mail list: I will support whatever our users want.

@dpwiz
Copy link

dpwiz commented Jun 21, 2015

Can we at least sweep generated code under src?

@gregwebs
Copy link
Member

Definitely. I am still planning to merge some of these changes.

@cdepillabout
Copy link

Sweeping the generated code under src would be a good start. 👍

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.

4 participants