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

Feature/go conventions #49

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open

Conversation

marcel-aan-zee
Copy link

renewed the Go code conventions

go/README.md Outdated Show resolved Hide resolved
go/README.md Outdated Show resolved Hide resolved
go/README.md Outdated Show resolved Hide resolved
go/README.md Outdated Show resolved Hide resolved
go/README.md Outdated Show resolved Hide resolved
go/README.md Outdated Show resolved Hide resolved
go/README.md Outdated

Every project must have a makefile.
(You can look at other projects for an universal working version)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we standardize the make commands? example would be the npm run start and npm run build

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Describe make file "standards"

go/README.md Outdated
## project structure for an API / Web application

main

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why start is necessary, start should be the root.

project structure for an API / Web application

main / start

  • business / app (separation of concerns)
    • domain (e.g.: users, cars, chairs etc. etc.)
      • handlers
      • middleware (local middelware)
      • models
      • queries (if you work with a database)
      • repositories
      • services
      • validators
  • config
  • datamodels
  • helpers
  • middleware (global middelswares)
  • web / server

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

capWeb.Authenticate > capilaMiddleware.Au....

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

config
go > src folder
dist

go/README.md Outdated Show resolved Hide resolved
go/README.md Outdated
## Variable naming

A variable inside a function can have a short name like "a", if it is clear what it is used for.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Variables should always explain what it does
Like in this example:
assert := asserts.New(t)
assert.Equals("Check", someVar)

or in a loop, like in this example:
for key, user := range users {
println("key:", key)
println("val:", user)
}

In any other case, it is wise to name the variable after the thing it contains.
If is can hold a collection of things, name it plural, like "users".
If it can only hold one value, name it singular, like "user".
if it is a boolean var, start it with something like "has", or "is".

which is mostly used for checking if an element is present in a map.
It's also common use, when a variable name holds an abbreviation, that the letters are all capitals, or all lowercase.
like "SQLStatement" or "sqlStatement"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion. Let's use this instead.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, what's the convention on naming const variables?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eerste stuk overnemen

go/README.md Outdated Show resolved Hide resolved
go/README.md Outdated Show resolved Hide resolved
go/README.md Outdated Show resolved Hide resolved
go/README.md Outdated Show resolved Hide resolved
go/README.md Outdated Show resolved Hide resolved
go/README.md Outdated Show resolved Hide resolved
go/README.md Outdated Show resolved Hide resolved
go/README.md Outdated Show resolved Hide resolved
go/README.md Outdated Show resolved Hide resolved
go/README.md Outdated Show resolved Hide resolved
(and remove the comments)

Before pushing the final update to git, make sure that the fork isn't in use anymore

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Standard behavior of go mod places the dependencies in your $GOPATH, go mod vendor puts them in a vendor folder inside your project, running go build -mod vendor uses the dependency put in the vendor folder. This further isolates the project from the globally installed dependencies

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

look at 1.13

go/README.md Outdated
## Code Quality

I wrote a lint tool, which also checks for cognitive complexity.
Copy link

@rickvandermeij-aanzee rickvandermeij-aanzee Aug 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess sonarQube will be sufficient. But can be used for all languages, so maybe move to general?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to know how the sonarcube linting can be changed. (is the change of the configuration a project setting that can be exported/imported (and can be checked in with the project itself?)

go/README.md Show resolved Hide resolved
go/README.md Outdated Show resolved Hide resolved
go/README.md Outdated
/validators

##
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like in the last commit the title 'Makefile' was removed (intentionally?) and an empty title ("##") was added. I would suggest adding a title or removing the empty title :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

## Project structure for an API / Web application

Root folder
Copy link

@rickvandermey rickvandermey Dec 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wellicht nog even goed kijken naar de MD structuur, leesbaar is het nu allerminst, geldt voor de volledige MD file

It is nice to start a linter now and then.
Make check, or make check2 will run the linters and will check your files for "mistakes".
Try to improve your code with the hints you get from them.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lint rules should be core. Without the correct 'style', the linter should fail with error code, thus the pull request cannot be merged

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.

9 participants