Organizing files into sub-packages

[brendensoares]

@revel/core What do you think about organizing the go files in the root directory into sub-packages?

I tried simply moving them into sub-folders, but in order for them to share the revel package they have to be in the same folder (this is odd and slightly irritating).

We should be able to do this without breaking compatibility and without introducing circular dependencies. Doing this should increase our separation of concerns and hopefully improve code quality through clear lines of division and unit testing.

I also feel that reorganizing into sub-packages will make contribution to Revel more inviting to the Go community and/or new comers.

[ianonavy]

As a new contributor, I agree that organizing the source files into subpackages would make it easier to focus on one issue.

[brendensoares]

Thanks @ianonavy and welcome! Glad to have you :)

@revel/core any thoughts?

[pushrax]

Seems reasonable, though how would we not break compatibility? Keep the common public interfaces in the root package, and proxy to the subpackages?

[landaire]

It'd be nice to retain compatibility but I don't think that should be too large of a concern. We haven't yet released v1.0, so it should be assumed things will break.

[brendensoares]

Yes, proxying/exposing the needed functionality would be a good solution.

I don't think the currently exposed interface would perceivably change. The restructuring would be only internal. Is there something I a may be overlooking? Perhaps the only way to be 100% certain is to give it a go.

[brendensoares]

Checkout the revised roadmap and specifically the "Create subpackages" list item. Does everyone agree that this list is reasonable and complete? Are there any other Revel components that should be made into sub-packages?

[NitroKKX]

Pardon my ignorance, but what is subpackages, and how is that different then placing the files in seperate folders ?

[brendensoares]

@NitroKKX when I say "subpackage" I mean exactly what you are thinking of: a subfolder inside the Revel core package that is then imported by the top level Revel code. It's a way to organize Go code since in Go each folder must be its own package.

To start, these subpackages will be internal only.

[ghost]

Are there any other Revel components that should be made into sub-packages?

@brendensoares, what as for util package?

[brendensoares]

@AnonX that's a possible package, but it seems it might be better as a more specific set of util packages instead of some catch all package. What do you think?

[NitroKKX]

Thanks for the explanation @brendensoares , I hadnt heard it termed subpackages before and just wanted to ensure I understood you properly. Shouldnt field and i18n be moved to a subpackage as well ? watcher should be moved into cmd as well since it is only relevant there too. Not sure about some of the other subpackages, since there are a lot of cross reference between them - like router, server and controller are pretty tight and may be best to leave them together in the revel package. The rest look good.. Just my (c)(c)

[brendensoares]

@NitroKKX Good point about moving the watcher to cmd. As for the coupling of router and server, I'll have to look closer to see what can be done, but separation of concerns and modularity are the high level goals. We want to have common interfaces that must be present without knowledge of implementation details such as the router being our current custom DSL format or YAML or whatever.

[ghost]

@brendensoares, Looks like params and utils should be turned into subpackages as well. Otherwise there will be import cycle. Because they are used by the sub-packages you have included in the roadmap and belong to revel package. So, revel imports sub-packages which import revel.

And I'd like to clarify what naming convention could be applied to params subpackage. For example, in case of Binder it is binding. So, we can use it as binding.Binder. What as for Params?

[brendensoares]

@AnonX isn't Params also related to binding? Perhaps we should combine it into the binding sub-package?

[ghost]

OK, I agree. @brendensoares, there is one more naming issue. How can the package Controller be called? There are already Controller struct and NewController method inside. So, controller.Controller is against golang convention. What do you think? controllers (plural) is the only thing that comes to my mind.

Let's make a decision on naming of every subpackage in advance.

• TestSuite -> testing
• Binder -> binding
• Router -> routing
• Server -> servers?
• Validation -> validations?
• Watcher -> watchers?
• Session -> sessions?
• Controller -> controllers?
• Filter -> ...? (We have both Filter and Filters inside)

[ghost]

@brendensoares And could you please clarify the criteria for extracting a component into subpackage. For example, we are going to turn Session filter into a subpackage. But at the same time there are no i18n, panic, flash, interceptor, and action filters in the roadmap. Don't forget we also have CSRF which should be a filter as well. I think there should be some logic and consistency behind the structure of the project. But I do not quite understand it now, to be honest.

[pedromorgan]

Where are we now with this..

I think there are inbuilt packages/modules eg jobs, testrunner
and then third party

?
its a config issue ??
http://revel-docs.daffodil.uk.com/manual/modules.html

and a manual for others..

maybe we need revel create mod to do that ?

[brendensoares]

@pedromorgan see the Roadmap for v0.12 and specifically the "Create sub-packages" checkbox.

I would like to create a few more internal packages, but @AnonX brings up some good points that I haven't thought through fully.

@AnonX my high level goal is to have a clear separation of concerns. I'd like to do this without affecting the exported interface that Revel devers see. I want it to be a purely internal change. I will admit that sub-packages may not be the correct solution for each of the items listed above and I am ready to find more reasonable solutions if we can think of them.

As far as criteria for creating a sub-package, I'd say if a segment of code is independently functional and complex enough (say ~300 lines of code or more) then we should seriously consider its re-organization. The criteria is somewhat subjective though.

PS - I am inclined to split v0.12 into 2 releases as it's too large currently. It's more important that we keep consistent momentum than trying to release large improvements. Agreed?

[brendensoares]

@AnonX it just struck me that we may not be understanding sub-packages the same way. Not all that I've suggested on the v0.12 Roadmap should be external packages. Internal, independently testable packages are more appropriate than external for most, if not all, of the remaining items. Agreed?

[ghost]

@brendensoares I'm asking about the criteria just for practical reasons. We have init.go with the following content:

revel.Filters = []revel.Filter{
    ...
    revel.RouterFilter,
    revel.SessionFilter,
    ...
}

I, as a user of the framework, expect that every standard filter will have predictable import path. For example:

import (
    "github.com/revel/revel/routing"
    "github.com/revel/revel/session"
)

So the filters above can be used as:

    ...
    routing.Filter,
    session.Filter,
    ...

If import paths are different I feel like something is wrong with consistency:

revel.Filters = []revel.Filter{
    revel.PanicFilter,
    routing.RouterFilter,
    revel.FilterConfiguringFilter,
    params.Filter,
}

I am inclined to split v0.12 into 2 releases as it's too large currently. It's more important that we keep consistent momentum than trying to release large improvements. Agreed?

👍

Internal, independently testable packages are more appropriate than external for most, if not all, of the remaining items. Agreed?

By external you mean packages in separate repos? If so, yes I agree with you. Now I'm not even sure that creation of a separate modules and samples repos was such a great idea. It turned out to complicate the process of testing without bringing any additional value.

[brendensoares]

@AnonX to your last point, the value I see is that it provides a place to put community driven modules and samples and alleviates the clutter in the core library. It does aggravate the package management issue aka version dependency, but that's a good thing; we need to decide a solution for that ASAP.

In regards to splitting the release, any good ideas on a release theme to put on the Roadmap?

Lastly, I didn't expect a dever to have to import any of the internal sub-packages, but maybe that was an oversight on my part. The session package has more than just a filter; it has/needs an interface to allow other backend drivers i.e. Redis, Files, In-memory etc. We could put the revel.SessionFilter in a filters sub-package and simply have it import a subsequent session package that contains the interface and other backend drivers. That way the revel package still owns the filter interface implementations and we can further modularize independent code i.e. sessions, routing, flash panic/error handling. Agreed?

Either way, it would be productive if we could chat directly for 30 minutes to outline RFCs for each of the remaining code re-organization efforts aka sub-packages.

[brendensoares]

@NitroKKX I've chatted with @AnonX and we decided it's best to keep watcher in the revel repo to ensure watcher functionality when not using the revel command.

Here is the outcome of our chat as far as new sub-packages:

• sub-package naming convention vs Revel API exported attributes
  • testing
  • binding
    • decided: combine binder and params into 1 package
  • routing
    • decided: move router.go
  • decided: no server
  • validation
    • decided: go ahead and move it to subpack
    • decided: Validation to validation.Context
  • watcher
    • decided: Watcher to watcher.Notifier
  • session
    • decided: session.Session to session.Values
    • decided: SessionFilter to session.Filter
    • merge flash.go to session i.e. FlashFitler with session.Filter
  • controller
    • decided: Controller to controller.Context and controller.NewContext
    • decided: pass in config.Context instance to NewController
  • filter
    • decided: remove initial var Filters values and require app/init.go to set them or panic
    • decided: Filter to filter.Type
    • decided: Filters to filter.Stack
  • config for app config...needs eventual abstraction for backend drivers
    • decided: MergedConfig to config.Context
    • decided: NewEmptyConfig to config.NewContext
    • decided: LoadConfig to config.LoadContext
    • need a revel/cmd issue
  • should make sub-packages more independent of Revel in the future

[NitroKKX]

Makes sense to me. Thanks

On Sat, Jan 3, 2015, 11:38 PM Brenden Soares notifications@github.com
wrote:

@NitroKKX https://github.com/NitroKKX I've chatted with @AnonX
https://github.com/anonx and we decided it's best to keep watcher in
the revel repo to ensure watcher functionality when not using the revel
command.

Here is the outcome of our chat
https://github.com/revel/revel/wiki/Meetings-2015-01-03 as far as new
sub-packages:

• sub-package naming convention vs Revel API exported attributes

  • testing
  • binding
    • decided: combine binder and params into 1 package
      • routing
    • decided: move router.go
      • decided: no server
  • validation
    • decided: go ahead and move it to subpack
    • decided: Validation to validation.Context
      • watcher
    • decided: Watcher to watcher.Notifier
      • session
    • decided: session.Session to session.Values
    • decided: SessionFilter to session.Filter
    • merge flash.go to session i.e. FlashFitler with session.Filter
      • controller
    • decided: Controller to controller.Context and
      controller.NewContext
    • decided: pass in config.Context instance to NewController
      • filter
    • decided: remove initial var Filters values and require
      app/init.go to set them or panic
    • decided: Filter to filter.Type
    • decided: Filters to filter.Stack
      • config for app config...needs eventual abstraction for backend
        drivers

   - decided: MergedConfig to config.Context
   - decided: NewEmptyConfig to config.NewContext
   - decided: LoadConfig to config.LoadContext
   - need a revel/cmd issue
  

  • should make sub-packages more independent of Revel in the future

—
Reply to this email directly or view it on GitHub
#614 (comment).

[brendensoares]

After trying to combine params and binder into binding I've decided they are separate concerns and should be packaged separately.

binder's input is a string and its output is a Go variable. I'm wondering if binder is the most appropriate name for this. It seems pack and unpack makes more sense than bind and unbind. It's main purpose is the conversion of strings to and from variables, right? In my experience, binding has to do with event processing more than data conversion. Thoughts?

So, it seems we'll need a params sub-package after all @AnonX. It's input is a net/http Request and its output is a key/value list of variables. I can't think of a better name for the package or any sort of *ing equivalent. I'm open to ideas. Same goes for binder, but I don't mind keeping binding.

[notzippy]

Rather than binding why not use encoding ? That is the standard used in golang for json and xml translations from strings to golang objects. It should be applicable here as well.

[brendensoares]

@notzippy that makes sense. What about params?

Any input @pushrax @AnonX ?

[ghost]

There are revel.TRACEs, revel.INFOs, revel.WARNs, and revel.ERRORs all around the code. Looks like we cannot go without a separate log package.

[notzippy]

For logs, we could define it as an interface in revel and implement it in a different package. From what I have seen with logging there are many different approaches to take, rather than attempt to have revel do it all why not allow someone to implement an interface to there favorite logging package and assign it in the config. By default revel would use the golang logger, (which would be in its own package)

[brendensoares]

@AnonX we can consider the use of dependency injection for challenges like this, but in this case, I don't think we even need the logger. It seems the better solution is to return errors where applicable instead of logging the errors directly. Revel can do what it wants with the errors. Agreed?

@notzippy the new logger interface is planned for v0.15. It's a need, but not as important as other items.

[ghost]

@brendensoares Yes, good idea. But sounds like a lot of work.

[brendensoares]

@AnonX I'm almost ready to push the PR for review. I have one last Params dependency in binding to solve and then I can show you my progress.

[ghost]

@brendensoares I'm looking forward to it.

[brendensoares]

#872

[jeevatkm]

I agree with organizing, if possible making independent as possible.

But my question is about naming and structure, for example:

import (
    "github.com/revel/revel/routing"
    "github.com/revel/revel/session"
)

Instead, it's better have structure/name like this:

import (
    "github.com/revel/log"
    "github.com/revel/routing"
    "github.com/revel/session"
    "github.com/revel/auth"
    "github.com/revel/csrf"
    "github.com/revel/db"
    "github.com/revel/testing"

    // etc...
)