Namespacing

[repomaa]

This is a just a suggestions but I thing it would be much cleaner and less error prone if all generated code would be namespaced at least to the application name. Even more preferable in my opinion would be further namespacing by Models, Pages, Actions etc.

[jwoertink]

I just ran in to 2 issues where namespacing would have helped. Working on an app for building out a computer with specs, and I wanted a model named System. That's a module in Crystal, so you can't have a class named System. The other is an app where you can plan to go to fun things where a model would be named Event, but since that exists in crystal, you can't use that name.

Generating apps with a default namespace could help to solve these issues, I think. I'm not sure about the secondary level of namespace, but at least a top level would help.

[jkthorne]

I ran into the case with event and it threw me off. It was very confusing.

[paulcsmith]

@wontruefree It looks like this has been fixed. When did you run into the error with Event? Was it awhile ago or recently?

[paulcsmith]

This is a good point. It would also helps with class collisions because you'd always need to preface it with the namespace render Pages::Users::Index

I have a few concerns/questions with this approach:

1. What if you want to namespace by a domain. Would it be Admin::Actions::Users::Index or Actions::Admin::Users::Index? I'm guessing the later (start with Actions) but is that strange? This would definitely make it harder to use "concept" based file structure if we decide to go that way Resource/context folder structure instead of structure based on file type #642
2. It is a bit extra to type. Probably not a big deal, but worth addressing.
3. I'm not sure how this would work with models because queries and forms inherit from the model. class Queries::User < Models::User::BaseQuery seems a bit strange :S

Any other thoughts/concerns with this approach? Any thoughts about this @edwardloveall?

[paulcsmith]

Actually it looks like we can define top-level classes from within a class 🤯

class User
  class ::User::BaseQuery
  end
end

User::BaseQuery.new # Works

So we could still have class Queries::User < User::BaseQuery but this seems a bit too magical ⚡️. Thoughts?

[jwoertink]

I'm not particularly keen on the secondary level namespacing idea, though I can see how it could be helpful. What I was seeing was something more like:

module MyBlogApp
  class Admin::Users::Index < BrowserAction
  end
end

module MyBlogApp
  class UserQuery < User::BaseQuery
  end
end

In my use case, I could then do

module FunApp
  class Event < BaseModel
    table :events do
    end
  end
end

This makes FunApp::Event an Avram Model, and ::Event a crystal core class.

[paulcsmith]

Ah I see. One issue I have with this is that it makes guides/tutorials strange because some people use MyApp some use their real app name, etc. Also some apps names are super long! If we did do app based namespacing, I'd suggest we default to App. The guides would be simple, it's short, etc.

[jwoertink]

I'd be ok with a simple module App as the parent namespace. It would make docs consistent, and no guess work for generating stuff.

[z64]

I'm wholly unfamiliar with Lucky's codebase, but I read this issue and it sounded similar to a problem I had, so thought I would bring it up:

I don't know what would be appropriate for Lucky, but when I ran into this I solved it by using a simple marker annotation to collect classes at compile time that are meant to integrate with the app.

module MyWebsite
  @[Lucky::App]
  class Blog
    # ..
  end
end

Anyway, just an idea 🙂

[jwoertink]

I still don't understand annotations much. What would this do exactly? With lucky, you'd have something like

class Event < Avram::Model
end

but since you can't have a class named Event in the global scope, we would need to namespace it. Could we use annotations to solve that?

[repomaa]

Hm any concluding thoughts? module App as parent namespace? Would you be willing to accept a PR that would implement this for the generators?

[jwoertink]

I've actually started using very custom namespaces because I have to share models between apps. I have 2 different models both named Site which connect to different databases, so they are each namespaced. My thought is this is probably a case by case scenario.

If we do decide to add this in, then I think default should be no namespace, and then some flag or maybe just the syntax itself.

lucky gen.model User                # generates just class User
lucky gen.model App::User      # generates class App::User
lucky gen.model User -n App  # maybe I don't like this afterall... but same App::User

But I'll defer to @paulcsmith on this one.

[wout]

While not 100% related, I also ran into a namespace issue just now. One of the sections in my app is under /settings, so I have a Settings::Index. But this clashes with Lucky::FormHelpers::Settings. Explicitly defining it at the root (e.g. ::Settings::Index) works, but it's not as nice.

[paulcsmith]

@wout That is a great idea. We should also see if Crystal can namespace some of the common class names like Event to Crystal::Event

[jwoertink]

Those are actually already under the Crystal namespace. https://github.com/crystal-lang/crystal/blob/master/src/crystal/system/event_loop.cr#L23 I haven't tried in some time, but it's possible that the crystal namespace was just leaking out globally (i.e. ::Event == Crystal::Event). Not totally sure. Maybe I will try making another app with those classes to see what happens. I should also note that when I originally tried, Lucky wasn't really a thing yet 😛

[paulcsmith]

Ohhh yeah. Just tried it. They must have fixed that already! 👍

[paulcsmith]

I like the idea of checking when generating a class, but so far I don't know of any. I knew Event was one, but looks like that was fixed. And Settings was from Lucky and we can fix that. So If someone runs into another collision I say we add some kind of check in the generator!

[wout]

Haven't seen any other collisions so far.