Skip to content

Conversation

@debpal
Copy link
Collaborator

@debpal debpal commented May 9, 2025

Updated the CSV file reading logic in the FileReader.__init__ function, linted the entire pySWATPlus package, and added a GitHub Actions workflow to enforce code linting using flake8.

@zepholus
Copy link
Collaborator

In linting.yml, you're setting --max-line-length=127 for flake8, but in setup.cfg, rule E501 (line length) is being ignored. This makes the --max-line-length setting ineffective.

It's totally fine to enforce a max line length, but it's a bit unclear what the intended limit is. Is 127 a deliberate choice? I’ve typically seen values like 88 (Black’s default) or 100 in other projects (it's my first time linting, so I don't know which one is better).

@debpal
Copy link
Collaborator Author

debpal commented May 11, 2025

In linting.yml, you're setting --max-line-length=127 for flake8, but in setup.cfg, rule E501 (line length) is being ignored. This makes the --max-line-length setting ineffective.

It's totally fine to enforce a max line length, but it's a bit unclear what the intended limit is. Is 127 a deliberate choice? I’ve typically seen values like 88 (Black’s default) or 100 in other projects (it's my first time linting, so I don't know which one is better).

@zepholus Yes, you're absolutely right — thanks for pointing that out! I had copied the linting config from another project and didn’t fully align the settings, so the --max-line-length=127 is currently ineffective due to E501 being ignored.

Since the GitHub Action passes, we can go ahead and merge this pull request. After that, feel free to push a follow-up change based on your preference — either enforce a specific line length (e.g., 88, 100, or 127), or keep ignoring E501. Totally up to you!

@zepholus zepholus merged commit de90e73 into swat-model:main May 11, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants