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

use luacheck #343

Closed
wants to merge 6 commits into from
Closed

use luacheck #343

wants to merge 6 commits into from

Conversation

fperrad
Copy link

@fperrad fperrad commented Feb 2, 2019

some minor improvements found by luacheck.

@elliottslaughter
Copy link
Member

elliottslaughter commented Feb 4, 2019

Thanks for this. Are there instructions for running luacheck?

My main concerns with this are:

  1. Like any formatting tool, the first run will result in large diffs that will definitely conflict with any other outstanding changes. Support for PUC Lua #320 is the main one I'm aware of right now, which is unfortunately not really in a state to be merged. There is also the develop branch, but it's unclear how much of that we want to keep. This mostly means it may take longer to get this in because I'd rather not fight through massive merge conflicts in the outstanding branches.

  2. There are some changes here that seem more controversial, e.g. removing parameters outright when they're not used in the body of the function, or replacing meaningful variable names with _ when they're not used. Variables like i in loops don't provide much value anyway, and it's fine to replace those with _, but things like the cudahome argument that got removed seem to communicate meaningful information and I'd reluctant to remove it unless we're really 100% sure that parameter isn't ever going to be needed in this function.

There was one test that failed that I wouldn't have expected to fail (the non-CUDA macOS test), I'm rerunning this now to see if it's deterministic or not.

@fperrad
Copy link
Author

fperrad commented Feb 9, 2019

I do a full rework of this PR.

Travis CI shows 1 failure with the config : LLVM_CONFIG=llvm-config-6.0 CLANG=clang-6.0
I cannot able to reproduce the issue.
But I suspect the commit "unused argument", so I remove it.

I also remove 4 last commits about whitespaces, there could create conflict with others PR.
I will put them in another PR only about formatting code.

As linter, luacheck is highly configurable tool which produces warnings.
There are different way to fix them:

  • fix the code
  • in a configuration file, disable the warning at project or file level
  • annotate the code with a specific comment which locally disables one warning (not my prefered way)

All warnings haven't the same criticity/severity.
The luacheck documentation sorts in these categories:

  • global variables (1xx)
  • unused variables (2xx) and values (3xx)
  • shadowing declarations (4xx)
  • control flow and data flow issues (5xx)
  • formatting issues (6xx)

With the current setting (see .luacheckrc), there are 0 warnings.

So, the next step is to call make luacheck in the continous integration, in order to prevent the coming of new warnings.

This pull request was closed.
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.

2 participants