clearlinux / clr-installer

Clear Linux* OS Installer
GNU General Public License v3.0
92 stars 42 forks source link

Lint: Add a target to check source line length #700

Closed karthikprabhuvinod closed 4 years ago

karthikprabhuvinod commented 4 years ago

Signed-off-by: Karthik Prabhu Vinod karthik.prabhu.vinod@intel.com

Changes proposed in this pull request:

mdhorn commented 4 years ago

Thank you for enabling this linter. Do we really want to force things down to 80? Maybe 100? We should have a discussion with the mixer team. Do we only care about go files only? Maybe there are good reasons for an occasional long line? -l 100 -g -e "TODO|FIXME"

karthikprabhuvinod commented 4 years ago

@mdhorn : So I think the default is 120. Thats we are using. Since you use vim, this will help you a lot. Also makes reviewing and reeading code easier on github.

Do we only care about go files only? Good question. I would opt choosing all code to follow it. Somehow using a go linter for all source code files

Maybe there are good reasons for an occasional long line? There were places I didnt want to do the line shortening like in storage_test where we have json output which looks exactly like a replica from lsblk -J -O /dev/sdx.

mdhorn commented 4 years ago

@mdhorn : So I think the default is 120. Thats we are using. Since you use vim, this will help you a lot. Also makes reviewing and reeading code easier on github.

Do we only care about go files only? Good question. I would opt choosing all code to follow it. Somehow using a go linter for all source code files

Maybe there are good reasons for an occasional long line? There were places I didnt want to do the line shortening like in storage_test where we have json output which looks exactly like a replica from lsblk -J -O /dev/sdx.

The Git page says the default is 80 characters, but I didn't test this. I sort of like the suggestion of 100 characters and excluding lines with TODO or FIXME -l 100 -e "TODO|FIXME" But we should discuss this in IRC with the mixer team so we can agree on a team standard.

karthikprabhuvinod commented 4 years ago

The Git page says the default is 80 characters, but I didn't test this. I sort of like the suggestion of 100 characters and excluding lines with TODO or FIXME -l 100 -e "TODO|FIXME" But we should discuss this in IRC with the mixer team so we can agree on a team standard.

@mdhorn I read the golangci documentation, to override the defaults, we will need to create golangci.yaml or json file to over-ride options for individual linters!

karthikprabhuvinod commented 4 years ago

@mdhorn @reaganlo 1.) I searched and thought hard, but i couldn't find a way to exclude on basis of exclude strings we define. Looks like --exclude is used to exclude error messages stemming from error messages and not user defined tags. Instead we can use something like

//nolint: lll // WONTFIX //nolint: lll // TODO //nolint: lll // FIXME

It helps linter skip the smallest possible construct(method, var etc) and skip a specific lint test on it

2.) Also for tab-width check for lll works, but it creates lots of things to fix. I can still go and fix this but these will be all manual because lll has no auto-fix :(

@mdhorn: let me know

mdhorn commented 4 years ago

@mdhorn @reaganlo 1.) I searched and thought hard, but i couldn't find a way to exclude on basis of exclude strings we define. Looks like --exclude is used to exclude error messages stemming from error messages and not user defined tags. Instead we can use something like

//nolint: lll // WONTFIX //nolint: lll // TODO //nolint: lll // FIXME

If the standard is //nolint: lll, that should be enough to add to lines of code that are too long, but that we want to leave long intentionally.
Where does the //nolint: lll reside? On the line with the problem? On the line with the problem?

2.) Also for tab-width check for lll works, but it creates lots of things to fix. I can still go and fix this but these will be all manual because lll has no auto-fix :(

Personally, I think the tap value should be 4, but do we know the reasoning it was set to 1 by default?

karthikprabhuvinod commented 4 years ago

@mdhorn I added tab-width changet to 4..looks like there werent a whole lot of fixes.

mdhorn commented 4 years ago

Fixed in installer release 2.5.0 found in Clear Linux OS release 32980 or greater.