Adds --baseline

[sobolevn]

Closes #1274

[coveralls]

Pull Request Test Coverage Report for Build 2864

• 87 of 87 (100.0%) changed or added relevant lines in 6 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 100.0%

---

Totals
Change from base Build 2847:	0.0%
Covered Lines:	5525
Relevant Lines:	5525

---

💛 - Coveralls

[coveralls]

Pull Request Test Coverage Report for Build 2872

• 87 of 87 (100.0%) changed or added relevant lines in 6 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 100.0%

---

Totals
Change from base Build 2847:	0.0%
Covered Lines:	5525
Relevant Lines:	5525

---

💛 - Coveralls

[asottile]

some distributions compile out md5 so you'll probably want a stronger hash function

[sobolevn]

Sure! Any suggestions based on your experience?

[webknjaz]

sha256 should do

[sobolevn]

@lk-geimfari do you have any ideas about this PR? I would love to hear them 🙂

[sobolevn]

Ok, here's the problem I am facing:

1. I have this code in my file: x1 = f''
2. I record a baseline from it: it contains just this WPS305 Found f string hash
3. Next I refactor my code to be x0 = f''; x1 = f''
4. Baseline reports x1 = f'' and not x0 = f'' because the error message is the same and the first match wins

Is it really a problem? On one hand, I am reporting an invalid violation. On the second hand, I am reporting the exact same violation but in a different place. I am not sure.

[sobolevn]

I guess it is: because users won't be able to even find the new violation. And sometimes it is really hard to fix existing ones. Consider this example:

User has a too complex jones score for a line. It cannot be refactored easily. Because of reasons. Mostly legacy ones. And then user writes some new complex line in a new function before the existing one. What happens? The existing one is reported. But, user cannot touch it. It is complex and important. And he even cannot identify where is the new complex one?

I don't want to be in a situation like this! I need to refactor the existing solution to fix cases like this one. We should also make a decision: which violations are we reporting and which one are fine.

I will refactor this part:

        for (error_code, line_number, column, text, physical_line) in results:
            # --- patch start
            # Here we ignore violations present in the baseline.
            if self.baseline and self.baseline.has(filename, error_code, text):
                continue

Into something like this:

for (error_code, line_number, column, text, physical_line) in baseline.only_new_violations(results):

I would also change the baseline format: I would need locations and physical_line too.

[sobolevn]

You can start using it with just a single command!

Better:

You can start using our linter with just a single command!

[skarzi]

really helpful feature 👍

[skarzi]

Maybe some message should be logged that baseline file is missing and it is going to be created?

[sobolevn]

Sure 👍

[sobolevn]

Based on the feedback I am going to change some details about how this works:

• I am going to store metadata in the baseline file. Some people said that they wanted to see the last date baseline was modified and when it was originally created
• I am going to add --baseline-refactoring flag that will force us to remove at least one violation from the baseline
• I am going to make the baseline mutable: whenever we remove any violations from the source, baseline needs to shrink by the given size
• I am going to report that a new baseline is created
• I am going to store human-readable information about all violations, not hashes
• I am going to change how violations are reported. I will try to do an intelligent guess here based on line number, column, line text, error code, and message

[sobolevn]

Results:

• baseline is immutable. It was a bad idea: a lot of tools do run flake8 a lot: like vscode integration, pre-commit hooks, etc. It would a hell to debug what went wrong and why now this violation is reported
• --baseline-refactoring is very hard to implement, I will just leave it as is for now

[sobolevn]

So, I have news!

I wrote a fuzzy matcher for baseline entries. It even includes several steps of negative and forehead lookups on what lines are there to determine if this code is new or not.

But!

It is a very hard thing to explain. I ended up not understanding the rules myself. It is a hell to test! And I have decided to try exactly the opposite solution this time: minimalism.

We can have two states of code: touched or untouched.
It is pretty clear what "untouched" code is: when it is equal to what we have in the baseline.
"Touched" is way more interesting. Let me explain.

First of all, let's say we have this python file (which is baselined):

– – – – – –
– –
– – – – –
– – – – – –

One can add a new line on top of the file:

x x
– – – – – –
– –
– – – – –
– – – – – –

Which will make all other lines "touched". Or one can add a line in the middle:

– – – – – –
– –
x x
– – – – –
– – – – – –

This will make only part of the lines touched.

The same is applied when we are talking about removing a line:

– –
– – – – –
– – – – – –

And that's fine. Sometimes people do have to insert new and remove things within a legacy code.
We can handle these two cases by making only one change in our algorithm: we can ignore the line number.

If one touch any line in any other manner, then sorry: you have to refactor it or to create a new baseline.

This way we can achieve the correct and still useful behaviour. First round eliminates all exact matches, the second one ignores line numbers. All other violations are reported as new ones.

[Dreamsorcerer]

Suggested change

	than your baselined violations will be ignored.
	then your baselined violations will be ignored.

[Dreamsorcerer]

Looking forward to this, I was just starting to implement this myself after finding that flakehell is fairly broken for our code base.

The documentation only talks about a CLI argument. Will it work with the config file too? If I set up a baseline, I'm not going to want to run flake8 without the baseline..
I imagined having a baseline = file_name in the config, and then using an explicit --create-baseline or similar argument for creating/overwriting the baseline.

I think being able to conveniently overwrite the baseline in a single step is also helpful. As an example, as I'm trying to roll this out into our project, not everyone will be running flake8 immediately (this gives us a chance to test it out and find the right balance in our flake8 config). This means those of us using it will likely need to create new baselines frequently to accommodate other people's changes until we have everyone running flake8 and can enforce it in CI.

Is this likely to make it into flake8 directly in future? Seems like something that should be useful for flake8 in general.

[sobolevn]

Hi, @Dreamsorcerer! Thanks a lot for your interest!

The documentation only talks about a CLI argument. Will it work with the config file too?

Yes, it surely will! This is just how flake8 works, so I didn't specify this explicitly.

and then using an explicit --create-baseline or similar argument for creating/overwriting the baseline

Can you please help me in researching this approach vs deleting a baseline manually and creating a new one with --baseline turned on?

I was thinking that manual deletion makes it less magical. And you control this process with very understandable and clear actions. But, I would love to hear other arguments!

Is this likely to make it into flake8 directly in future? Seems like something that should be useful for flake8 in general.

Yes, we had a discussion about it in https://gitlab.com/pycqa/flake8/-/issues/602
I surely want to collaborate and provide my implementation as a starting point.

But, this is a very complex feature. So, I am not sure about its future in the core. You can ask @asottile about his opinion on this.

I am fine with both outcomes:

1. Core feature will be more tested, native, documented, and more well-known
2. Our own implementation will attract more people to wemake-python-styleguide directly. It is a cool unique feature to have!

I was just starting to implement this myself after finding that flakehell is fairly broken for our code base.

I would love to see your solution. Also, if you wish we can collaborate on this PR together. Because, I am struggling to find some free time to work on this. (Currently I am too busy writing returns)

[Dreamsorcerer]

and then using an explicit --create-baseline or similar argument for creating/overwriting the baseline

Can you please help me in researching this approach vs deleting a baseline manually and creating a new one with --baseline turned on?

I was thinking that manual deletion makes it less magical. And you control this process with very understandable and clear actions. But, I would love to hear other arguments!

Ha, I was thinking the opposite. :P
It seems to me that the behaviour would be inconsistent with normal flake8 usage, if running the same command twice in a row produces 2 different sets of output. If the user fails to read the documentation, I would expect some confusion here.

I think that an explicit create option could avoid any confusion, and slightly reduce friction in our use case where we expect to recreate it often. It could also potentially catch some unexpected effects, such as if the file were moved/renamed accidentally, then flake8 could give an error saying that the baseline file could not be found. Without that, they would suddenly get huge amounts of violations, and possibly not understand why (especially if they are not the person who created the baseline in the first place).

I was just starting to implement this myself after finding that flakehell is fairly broken for our code base.

I would love to see your solution. Also, if you wish we can collaborate on this PR together. Because, I am struggling to find some free time to work on this. (Currently I am too busy writing returns)

I was just extracting the baseline code from flakehell, and stripping out all the other functionality (it's all that other functionality which was breaking things for our project). So, I presume you've already seen that code. I had only just made a start on it, when I was pointed here.

Happy to help out where I can (we have a hack day in a couple of weeks, so I can probably spend a full day working on it then). I'll try and get your branch working locally and test it out sometime this week.

[Dreamsorcerer]

I was thinking that manual deletion makes it less magical. And you control this process with very understandable and clear actions. But, I would love to hear other arguments!

Just thinking a little more about being explicit. I think running rm filename and flake8 --create-baseline are both clear, explicit actions to the user. However, not just if the baseline gets moved by accident, but also if a flake8 config gets copied into another project, then your approach would result in the baseline being created implicitly with no clear user action. In the case of copying a config to another project, there's a good chance that was not the desired behaviour. --create-baseline would never be run implicitly.

[sobolevn]

I think running rm filename and flake8 --create-baseline are both clear, explicit actions to the user

@Dreamsorcerer yes, I like this idea! This will indeed make the process even more clear.

Feel welcome to contribute 🙂

[Dreamsorcerer]

@sobolevn I've got the branch up and running. Are your latest changes on this branch? You mentioned earlier about fuzzy matching instead of hashes, but I'm still seeing hashes in the baseline file. It's also not using the baseline file, it creates the file just fine, but still gives me all the violations when I run it again.

[sobolevn]

@Dreamsorcerer sorry, updated the branch with the latest local changes.

[Dreamsorcerer]

Thanks. Still doesn't seem to ignore the violations successfully, but definetely doing samething now. On second run, it produces a load of try, ignored, violations and candidates lines, which I'm assuming is some debug output, but still exits 1. Also, doesn't seem to run when used as a git hook.

So, looks like there's a couple of tasks I should start by looking at.

[Dreamsorcerer]

I'm going to have a go at this next week. I'm going to look at:

• --create-baseline (I'm finding plenty more edge cases where the implicit baseline doesn't work)
• Remove unused violations from baseline.
• Ensuring everything still works well when passing in specific files (rather than running on the entire project).
• Try to catch file renames.
• Remove or suppress the debug output.
• Look at a way for report generation to produce reports of the baseline.

[Dreamsorcerer]

OK, the fuzzy matching algorithm needs some work. I added a line into a file and 2 violations 100+ lines away reappeared. The only change is that the line number moved down by 1.

Will try and debug that one tomorrow.

[sobolevn]

@Dreamsorcerer thanks a lot for your time and effort! 👍

[Dreamsorcerer]

OK, opened up a PR with all the work done today and yesterday: #1471

I'll try and start using this for our project on a daily basis next week, so I can see if there are any issues that come up.

Some ideas for improvements (but shouldn't be considered blocking this feature):

• Looser fuzzy matching. e.g. If line number is the same and column number is <10 characters different and/or if physical line is 50% similar. Then we would be able to catch multiple small changes at once (e.g. a variable name being changed at the same time the line number is moved due to other code being added above).
• If a file is deleted, it won't get removed from the baseline currently, would be nice to clean that up.
• Support displaying the baseline, which can then be used with --statistics or flake8-html. This would involve overriding the run checks and passing the stored baseline directly to the report generation.

[sobolevn]

@Dreamsorcerer thanks a lot! I really appreciate you working on a release-blocking feature. 🙏

We can work on fuzzy-matcher during or after we implement the core features. As you wish.
Me, personaly, would prefer the second way 🙂

[ma-sadeghi]

Is this PR being actively worked on? Would be great to have baseline...

[Dreamsorcerer]

See #1817 (comment)

I think at this point, somebody would have to volunteer to convert the code to a dedicated flake8 plugin. You'll want to look at using this branch though: #1471
It's in a good enough situation to release.

[sobolevn]

No, --baseline won't be added with this PR. Reasons:

1. It does not detect code movements
2. It heavily depends on flake8's internals
3. It is hacky