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

Added Black and Flake8 including workflow #1114

Closed
wants to merge 6 commits into from
Closed

Added Black and Flake8 including workflow #1114

wants to merge 6 commits into from

Conversation

TimidRobot
Copy link
Contributor

@TimidRobot TimidRobot commented Jul 24, 2020

  • Added Black and Flake8 including workflow and updated tools/README.md:
    • psf/black: The uncompromising Python code formatter
    • Flake8: Your Tool For Style Guide Enforcement
  • Reformatted code with Black
  • Manually updated code to comply with flake8 (or skipped errors where it conflicted with Black)

Unit tests passed on my laptop:

=============================== test session starts ================================
platform darwin -- Python 3.8.5, pytest-5.4.0, py-1.9.0, pluggy-0.13.1 -- [...]
cachedir: .pytest_cache
rootdir: [...]/police-brutality/tools
plugins: cov-2.9.0
collected 11 items                                                                 

tests/test_data_builder.py::test_title_to_name_date[Title here | May 30th-expected0] PASSED [  9%]
tests/test_data_builder.py::test_title_to_name_date[Title here-expected1] PASSED [ 18%]
tests/test_data_builder.py::test_handle_missing_name PASSED                  [ 27%]
tests/test_data_builder.py::test_handle_name_with_multiple_pipes SKIPPED     [ 36%]
tests/test_data_builder.py::test_handle_missing_date PASSED                  [ 45%]
tests/test_data_builder.py::test_handle_weird_date_format PASSED             [ 54%]
tests/test_data_builder.py::test_handle_nonexistant_date SKIPPED             [ 63%]
tests/test_process_md_texts.py::test_handle_dc PASSED                        [ 72%]
tests/test_process_md_texts.py::test_handle_missing_location PASSED          [ 81%]
tests/test_process_md_texts.py::test_handle_missing_links PASSED             [ 90%]
tests/test_process_md_texts.py::test_more_than_200_records_found PASSED      [100%]

=========================== 9 passed, 2 skipped in 0.07s ===========================

@TimidRobot
Copy link
Contributor Author

Once this PR has been merged (or rejected if ya'll don't want it), I will submit a PR based on my fix-incomplete-python-tests branch.

@ubershmekel
Copy link
Collaborator

ubershmekel commented Aug 1, 2020

Hi @TimidRobot sorry for the slow response.

I'm not too quick to merge yet another formatting PR. I think the previous one used autopep8. Also I personally don't prefer ' vs " or vice versa so this style seems a potential for needless friction.

I was also trying to figure out how to get github actions to auto-format markdown edit pull requests. Which seems is impossible when the PRs are on a fork. As a result, we might end up moving the workflows to circleci. See #780

So it might take a bit more time than usual to decide on this PR. Sorry about that. If you have advice on the topic, I'd be happy to hear it.

@ubershmekel ubershmekel added the Engineering Changes our tools and data pipeline label Aug 1, 2020
@TimidRobot
Copy link
Contributor Author

Usually I find that style/syntax is only an issue when the convention is unclear. I like Black because it largely removes style/syntax as a discussion item in PRs. It becomes simply a matter of whether Black was run prior to committing (which the included workflow would indicate).

@TimidRobot
Copy link
Contributor Author

I also really appreciate that Black saves me work. I can leave complex data structures "ugly" and divergent from anyone's coding standard and let Black do all the work for me.

@emarcey
Copy link
Collaborator

emarcey commented Aug 11, 2020

We're looking into using another ci for the github actions, so we probably won't look at this until after that's done.

Thanks for the PRs though, I definitely like seeing some Black and Flake8

@TimidRobot
Copy link
Contributor Author

We're looking into using another ci for the github actions, so we probably won't look at this until after that's done.

Thanks for the PRs though, I definitely like seeing some Black and Flake8

I'm happy to update the PR. Would you like me to remove the GitHub Actions workflow?

@emarcey
Copy link
Collaborator

emarcey commented Aug 11, 2020

@TimidRobot What I might do is merge this branch it into another branch I'm working on. I have black on save, so that's why the merge conflicts are so big right now.

I know in your previous PR, you mentioned some code improvements you had in mind. I would say maybe focus on those and I'll try to get this pipeline merged into one of my branches when I have time.

@TimidRobot TimidRobot closed this Sep 3, 2020
@TimidRobot TimidRobot deleted the black-flake8 branch September 3, 2020 06:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering Changes our tools and data pipeline
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants