CI: Unify code_checks whitespace checking
State: closed

- [ ] closes #xxxx
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry

Unify test cases of #30467 #30708 #30737

Labels:
name: Testing
description: pandas testing functions or related to the test suite
name: CI
description: Continuous Integration

Comments:
For #30737. Is it possible to do this via regex? similar to the current code check for context manager:
https://github.com/pandas-dev/pandas/blob/master/ci/code_checks.sh#L157
> For #30737. Is it possible to do this via regex? similar to the current code check for context manager:
> https://github.com/pandas-dev/pandas/blob/master/ci/code_checks.sh#L157

Yes, for simple cases like

```python
with pytest.raises(ValueError)
```
or (For false positives)
```python
with pytest.raises(ValueError, match="foo")
```


But I want a "global" test case.

So even if there's an edge case, for example:

```python
with pytest.raises(
                       ValueError,
                       # Some comment
)
```

or (For the case of false positives)

```python
with pytest.raises(
                       ValueError,
                       # Some comment
                       match="foo"
)
```


Regex is very fast, and for a reason, regex is not looking in the ```ast```. But it's hard for me to predict the future edge cases, and write regex to handle them.

So I wrote a global test case that looks in the ```ast```.

When I detect the occurrence of ```pytests.raises```, I brute-force all the tokens in the logical line until the end of the logical line, and if I find the occurrence of ```match``` as a argument (```token.NAME```) I consider this logical line as good.

My point is, regex is nice for "simple things" but when you have so many edge cases, I'd like to stick to checking it via the ```ast```, because it's hard to check with regex what is a logical line.

P.S
If you're not convinced yet @alimcmaster1 I have some more edge cases that I couldn't found matching regex for them, If you succeed doing so I'd be more than happy to see the solution:) 
Agree with Will, do you mind leaving the name unchanged in this PR, even if it's not accurate, so we can review the new changes only. Then it's easy to just rename in a follow up, and get that merged quickly.

Just one early comment, I'm -1 on creating abbreviations like `STD`. I think it's worth having proper names that can be directly understood, even if they are longer.
> Any chance you can make the edits in place first without changing files?

Not sure what you mean by that @WillAyd , can you please be more specific?


Check my last comment
> Check my last comment

So just change the function names, back to the __very__ original?
No, sorry. The file name, so the diff will show what you changed in that file. 
Hello @MomIsBestFriend! Thanks for updating this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:





There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers: 

##### Comment last updated at 2020-03-23 09:47:10 UTC
@datapythonista @WillAyd 

I added a "sample" test file, just for you to tell me if I'm on the right track or not.

Do you have any thoughts/suggestions?
@datapythonista Would you recommend "Squashing" some of these tests using ```pytest.mark.parameterize```?
gentle ping @datapythonista  
ping @datapythonista
@WillAyd over to you
This is... a lot.  It'd be great if one of the upstream tools (flake8/black/...) did this, but I'm not sure it belongs here.  i.e. if I had to maintain this myself, I would just learn to be OK with the downsides of regexps.
updated the title for clarity, feel free to revert if I misunderstood
@jbrockmendel The new title is correct, but not fully correct, this PR is basically merging the test cases introduced in #30467 #30708 #30737.

And it's also adding to the CI checks the test case introduced in #30708

I can't think of a better name for this PR unfortunately.

> This is... a lot. It'd be great if one of the upstream tools (flake8/black/...) did this, but I'm not sure it belongs here. i.e. if I had to maintain this myself, I would just learn to be OK with the downsides of regexps.

@jbrockmendel I can see ```flake8``` __maybe__ adopting the ```unconcatnated strings```, but for the rest of the tests cases I can't see why a linter would adopt those, as it's a very specific ```pandas``` code style/nitpick, maybe a very opinionated linters such as ```pylint``` would use those.

(And do you know by any chance someone from ```flake8``` to ask if they would use it?
And I would love to see how they do it, I tried doing this code checks using the ```ast``` module, couldn't figure it out :/)

And about the regex, I completely understand why this PR seems like overkill when a simple regex can do the trick, for the most part. but isn't that the fact why we are using tools like ```black``` an ```isort```? so there will no questions if the code style right or not, there is always one correct answer and it is what the linter says, when we disagree with the decision of the linter for example in https://github.com/psf/black/issues/1051 we just create our own check (If it's not upstreamed, we do it here).

I agree having these checks in the pandas code base is not ideal. But I'd move forward with this PR, get the validation working better (we already have the validation, this is just improving and refactoring what we've got). And later on, we can consider moving this out of pandas, as we did with the docstrings.

@MomIsBestFriend can you merge master and see if the CI is green please?
I'm completely happy deferring to @datapythonista on this.  Thanks @MomIsBestFriend for taking this on.
Restarting azure
ping @datapythonista 
I will relook in a few days
@MomIsBestFriend sorry we've been slow with this. Do you mind merging master once more please? I'll merge this once CI is green.
@datapythonista I just merged master, I'll wait for another commit to be merged, before I can merge master again. 
Oh, sorry, I thought the CI was stalled, never mind then. Ping me once it's green if I don't merge it before. Thanks!
ping @datapythonista :)
Thanks @MomIsBestFriend, great job.

Can you follow up renaming the file please?