search

inaka / elvis_core

The core of an Erlang linter
Other
60 stars 56 forks source link

Warnings for empty folders triggered when using "elvis git-branch" #196

Open cdyb-kivra opened 3 years ago

cdyb-kivra commented 3 years ago

The warning introduced in commit 1cfbd982371caa276a575b1a965775830819c52e gets triggered when using the git-branch subcommand to elvis, and the branch being worked on lacks changed files in some configured directories. That's not very useful, and I assume was not intended.

paulo-ferraz-oliveira commented 3 years ago

Hi, @cdyb-kivra. Thanks for the report.

I'm not sure what "using the git-branch subcommand to elvis" means 😕, though.

In any case, when the warning is issued, is it correct? (i.e. are the folders empty or non-existing?)

It was, indeed, not a concern of mine when developing it to assume different behaviours using branches, but I can revisit that.

Since it's a warning, and not an error (we did discussed it being an error, but didn't go that route) what would you suggest we do differently?

cdyb-kivra commented 3 years ago

The git-branch subcommand is used to only run elvis on those files that have been changed compared to a given git branch. So, for example, my elvis.config points at three directories, src, test and include. Most of the time I work on a feature branch and change files in src and test. I then run the elvis git-branch command to get it to check only those files I have actually edited (compared to our main branch). With the latest elvis version, I then always get a warning about there being no files in include and that I should update my configuration. Which is not true. Since it's a warning, it's not really a problem, but it is annoying and it teaches to ignore warnings from elvis.

I haven't looked at how this is implemented, but offhand it feels like the check that triggers the warning should be applied before the git-branch filtering is done. Or, possibly, that the warning should just be suppressed entirely when the git-branch subcommand is running.

paulo-ferraz-oliveira commented 3 years ago

it is annoying and it teaches to ignore warnings from elvis

I agree it's no good.

I didn't know about elvis git-branch. I use elvis only as a plug-in, which is why I didn't consider that "edge" case.

I'm pulling @elbrujohalcon to this issue, since this'll probably also include changes to elvis.

I haven't looked at how this is implemented, but offhand it feels like the check that triggers the warning should be applied before the git-branch filtering is done. Or, possibly, that the warning should just be suppressed entirely when the git-branch subcommand is running.

This is the bit that seems to need some kind of tweaking in the code. elvis is "aware" of elvis_core but not the other way around, though they should play nice.

elbrujohalcon commented 3 years ago

It's been ages since I last saw git-branch anywhere. I think that, instead of myself, you should pull @jfacorro here, @paulo-ferraz-oliveira. Sorry, but I can help as much or even less than you on this topic.

cdyb-kivra commented 3 years ago

At the company I work for (Kivra), we use elvis git-branch as part of our CI/CD pipeline. So it's not just me personally :-)

paulo-ferraz-oliveira commented 3 years ago

Sure. Let's wait for @jfacorro's input, then. We either have to silence elvis_core via an API variable, an option or, ... another thing (?)