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.

Sign up for GitHub

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

Jump to bottom

Provide (more) consistent linting and formatting with black and flake8 #251

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Provide (more) consistent linting and formatting with black and flake8 #251

wants to merge 1 commit into from

Conversation

maltekuehl
Copy link

Resolves #245.

The goal of this PR is to provide the necessary basis to facilitate contribution from different people to the python code in the future while at the same time keeping disruption to the existing workflow and changes to a minimum.

Features of this PR:

  • Adds a requirements.txt to the scripts folder to keep track of dependencies
  • Provides a Makefile to easily install the necessary dependencies and setup linting and formatting
  • Uses flake8 and black for linting and formatting inside the scripts
  • Adds most of the current linting problems to the .flake8 ignore list with a description of the problem. This prevents yellow and red flags from popping up all over the code in the IDE but allows for progressive removal of the flags and enhancement of the code style if desired.
  • Reformats the Python files using this setup
  • Includes the VS Code settings to enable linting and formatting

I've tried to do justice to most of the points raised by @ivan-aksamentov in his comment on #245 and tried to keep this as light and unobtrusive as possible but I'm aware that this is an opinionated solution that may require changes.

Debatable point may include:

  • Should the VS Code settings be included in /scripts/.vscode/settings.json since this is an IDE-specific feature?
  • Use of virtual environments for the setup
  • Use of second Makefile for setup: The reason behind this is to make linting and formatting completely optional and only part of the scripts folder. However other solutions might be providing a bash script for setup or integrating the make endpoint into the project's main Makefile.
  • Some imports from the code are not in the scripts folder, their paths could be added to python.analysis.extraPaths in the VS Code settings.json.
  • The requirements.txt file currently only contains the package names but not their versions, perhaps the output of pip freeze could be added here to add exact versioning.

Due to missing data it is also not possible for me to test all of the functions. Since only the formatting has been changed their functionality should not be affected but it would probably be good to test and confirm this anyway.

Happy to hear your feedback and to either move forward with this draft or reconsider the approach or certain aspects.

@vercel
Copy link

vercel bot commented Dec 30, 2021 •
edited

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/hodcroftlab/covariants/9WJR44XgrHjJJhbsDYQrpMpoCstz
✅ Preview: https://covariants-git-fork-maltekuehl-master-hodcroftlab.vercel.app

@ivan-aksamentov
Copy link
Member

ivan-aksamentov commented Jan 3, 2022 •
edited

Hi Malte @maltekuehl,

Thanks for looking into this. I like it!

I address the "Debatable points":

  • Should the VS Code settings be included [...]

    I don't have opinion on vscode config. I use PyCharm, so as long as it does not bother Emma, I am fine with it. I'd say let's give it a try.

  • Use of virtual environments for the setup

    Same. As far as I understand, deps can also be installed through

    pip3 install -r requirements.txt

    , so any environments are optional and they don't ask for money, so I'd say we keep it if it's useful for someone.

  • Use of second Makefile for setup

    Yeah, I'd prefer the simplest solution with the single makefile at the root.
    The scripts folder is somewhat cluttered already. Could you indeed move all the config files to the root of the project, so that they don't distract Emma too much?

  • Some imports from the code are not in the scripts folder, their paths could be added to python.analysis.extraPaths in the VS Code settings.json.

    Which imports do you have in mind? Is there a way to address this in the linter config, for cases when VSCode is not used?

  • The requirements.txt file currently only contains the package names but not their versions, perhaps the output of pip freeze could be added here to add exact versioning.

    Not sure if we have any version constraints. I'd say we keep them on latest unless proven that a freeze is needed.

Additionally:

  • Could you add makefile targets for each of the tools so that they can be invoked easily? For example:

    make lint
make lint-fix    # to apply auto-fixes, if any
make format   # to fix code formatting
...

  • I suggest we revert all the code changes for now. We would then merge only the config files, and synchronize with Emma, telling her what commands to run to apply the fixes. This is to avoid messy merge conflicts, because Emma keeps modifying the scripts constantly and it may also break some of the changes she might have in branches and stashes. The easiest way to revert is to pick files from the parent commit of this branch, copy them over to a safe place check out the tip of the branch again and then copy them back. This should remove these files from GitHub diff and resolve the existing conflict.

    Emma will then reapply the formatting fixes when it's convenient for her. And after formatting is fixed, we can proceed to picking on linting problems in small chunks. I'd like to decouple formatting and fixes, to make sure that any potential changes in code behavior can be spotted.

Really appreciate your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Consistent python style, linting and formatting for scripts folder
2 participants
@maltekuehl @ivan-aksamentov