fragmenta / fragmenta-cms

A user-friendly CMS written in Go (golang)
http://fragmenta.eu
MIT License
586 stars 70 forks source link

Pass lint, vet and consolidate. #8

Closed buddhamagnet closed 7 years ago

buddhamagnet commented 8 years ago

Love the fact there is a CMS in Go!

I have taken the time to go through the codebase and endure it passes golint, go vet, all fucntions and methods are commented correctly for godoc and made some changes to the way roles are queried to make it less repetitive. Hope this helps, especially if you'd like to attract contributors. I have the following settings in GoSublime if you use that:

"on_save": [{
      "cmd": "gs9o_open", "args": {
        "run": ["sh",
          "errcheck && godep go build . errors && godep go test -i && godep go test && go vet && golint ."],
    "focus_view": false
    }}],
    "autocomplete_closures": true,
    "complete_builtins": true,

Hope this helps! All tests are passing but I did notice the following which was already issuing a warning:

src/lib/mail/mail.go:46: cannot use renderContext literal (type *renderContext) as type view.RenderContext in argument to view.New:
    *renderContext does not implement view.RenderContext (missing Writer method)
src/lib/mail/mail.go:53: view.RenderString undefined (type *view.Renderer has no field or method RenderString)
kennygrant commented 8 years ago

Thanks, I'll have a look and see if I can merge it. In theory if just linter changes it should be fine, but I'll need to look at the roles stuff you mentioned.

Ideally before a pull request just put up an issue with your plans, so that we can discuss it first. Also try to separate out changes into small discrete ones (for example roles should be separate).

buddhamagnet commented 8 years ago

No problems @kennygrant will bear that in mind in future.

buddhamagnet commented 8 years ago

Looking good @kennygrant or want me to close it?

kennygrant commented 8 years ago

I'm still looking at it, there are a few changes that still need to be made if all the refs to Id are to be renamed like this (for example refs in templates). Leave it with me and I'll get it merged.

kennygrant commented 7 years ago

Thanks for your contribution.

I've taken in the suggestion to rename Id -> ID and Url to URL now, and I think it now passes the linter everywhere so all those code comments are done, but unfortunately I did this as a large refactor so I'm going to close this now. For future pull requests please try to tie them to an issue first, and keep them smaller so that they're easier to merge.