search

Yale-LILY / SummerTime

An open-source text summarization toolkit for non-experts. EMNLP'2021 Demo
https://arxiv.org/abs/2108.12738
Apache License 2.0
264 stars 30 forks source link

Troyfeng116/code styling test #25

Closed troyfeng116 closed 3 years ago

troyfeng116 commented 3 years ago
niansong1996 commented 3 years ago

I was just seeing this, the instructions in the readme are really nice!

I think we should have a go with this, let us wait on Zhangir and Murori to push their changes, then we will merge them to this branch and we will apply flake and black on all file and merge back to main, sounds good?

troyfeng116 commented 3 years ago

I was just seeing this, the instructions in the readme are really nice!

I think we should have a go with this, let us wait on Zhangir and Murori to push their changes, then we will merge them to this branch and we will apply flake and black on all file and merge back to main, sounds good?

👍 yeah would be best to wait until all outstanding changes are merged, and then run on all files to avoid conflicts. After that, we can sync up and figure out how we want to maintain code styling for future changes

troyfeng116 commented 3 years ago

@niansong1996 This branch should be ready to merge. Added pytest --flake8 with some ignore flags to the test suite; tests will fail if flake8 prints any error-level logs. Updated README; everyone should run black . and flake8. and even pytest --flake8 (copy full script from travis.yml) before pushing to PRs to ensure that code formatting passes. I've experimented with mypy for static type checking and there are so many errors, so I'd like to have this merged before moving on to type checking since each of these will create a lot of changes across the entire codebase as you can see from diffs in this PR

troyfeng116 commented 3 years ago

Also, I've turned off flake8 rule E501 in setup.cfg, which enforces max line lengths. Default is 80, but many of our docstrings and comments far exceed it, and black does not touch those when enforcing max line length. As long as we continue to run black FILE.py on all changed files, while comments and docstrings will exceed 80 chars, code will not. Kind of an annoying difference between black and flake8 line length enforcing standards. Let me know what you think, or if we should maybe enforce a 100-120 char limit even on comments/docstrings

niansong1996 commented 3 years ago

Thanks a lot for the effort, a couple of questions/comments:

I've experimented with mypy for static type checking and there are so many errors, so I'd like to have this merged before moving on to type checking since each of these will create a lot of changes across the entire codebase as you can see from diffs in this PR

I agree that we should merge this first, but can you confirm that all those changes are automatically generated and not manually done by you, right ;) ?

Also, I've turned off flake8 rule E501 in setup.cfg, which enforces max line lengths. Default is 80, but many of our docstrings and comments far exceed it, and black does not touch those when enforcing max line length. As long as we continue to run black FILE.py on all changed files, while comments and docstrings will exceed 80 chars, code will not. Kind of an annoying difference between black and flake8 line length enforcing standards.

I am not sure if I understand this, but do you mean that we can only enforce max line length for code with automatic reformatting and not for comments/docstrings?

troyfeng116 commented 3 years ago

I agree that we should merge this first, but can you confirm that all those changes are automatically generated and not manually done by you, right ;) ?

Actually mpypy does not actually write changes, only logs. So yeah, the changes would have to be manual lol

I am not sure if I understand this, but do you mean that we can only enforce max line length for code with automatic reformatting and not for comments/docstrings?

Sorry, and yes exactly, black enforces max line length but ignores comments/docstrings, and it will write to files. However, flake8 enforces max line length including comments/docstrings, and it does not write, only logs and errors. Since we have a ton of comments/docstrings exceeding 80 and even 120 chars, I've just disabled the max line length rule on flake8 (which we use in the test suite as pytest --flake8), but this means we should all run black before pushing code since line length won't be tested. At some point, we'll want to go through and make sure even comments/docstrings conform to max line length and then add the rule back

niansong1996 commented 3 years ago

I see. I think probably a good way is to enable flake8 and mpypy in our CI routine and output everything as errors. Then we let people know that we've also configured black to fix those automatically, but for static types, docstrings and comments, they would need to fix it themselves. I think it's better than "asking everyone to do xyz" before making the PR, what do you think?

troyfeng116 commented 3 years ago

I see. I think probably a good way is to enable flake8 and mpypy in our CI routine and output everything as errors. Then we let people know that we've also configured black to fix those automatically, but for static types, docstrings and comments, they would need to fix it themselves. I think it's better than "asking everyone to do xyz" before making the PR, what do you think?

Yeah agreed. So for now, to avoid another PR that's way too big, maybe we can do this:

Lmk what you think

troyfeng116 commented 3 years ago

After GH actions are merged and these tests and changes are moved to that new system, this PR can be closed.

niansong1996 commented 3 years ago

I like your plan, let's proceed that way.

Fixed a minor conflict at my own discretion, please check.