Skip to content

Conversation

@tschm
Copy link
Contributor

@tschm tschm commented Nov 10, 2025

  • moves linting to ruff
  • adds editorconfig
  • makes tests dependent on passing linting

Also fixes linting in some files (but not all).

@tschm
Copy link
Contributor Author

tschm commented Nov 10, 2025

@fkiraly I install an .editorconfig file and also deactivate the linting business (which will later be replaced with ruff)

@tschm
Copy link
Contributor Author

tschm commented Nov 10, 2025

@fkiraly this could be merged

Copy link
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Though I would not sequence it to include a full rework of linting but do it in one PR.

  • I agree with moving to ruff!
  • I think a single PR should replace current config with ruff, so there is no intermediate state where linting is off
  • the codecov change seems unrelated, should it be in this PR?

Otherwise, agreed with the direction, except that linting upgrade should happen in the same PR.

Generally, I would kindly like to request descriptive PR titles and descriptions of what a PR is doing. If you accidentally opened with an empty description, you can edit it (dots at the top right).

@fkiraly fkiraly added the maintenance Continuous integration, unit testing & package distribution label Nov 10, 2025
@fkiraly fkiraly changed the title Editorconfig2 [MNT] moving linting to ruff, adding editorconfig Nov 10, 2025
@tschm
Copy link
Contributor Author

tschm commented Nov 11, 2025

uvx pre-commit run --show-diff-on-failure --color=always --all-files

@tschm
Copy link
Contributor Author

tschm commented Nov 11, 2025

@fkiraly This is now using ruff. For now, I have deactivated most of my usual rules -- to avoid the code blood bath. You can execute ruff locally using "uvx ruff check ." I have also added ruff as a pre-commit hook.

Copy link
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

Instead of ruff.toml, can we move to pyproject.toml to have all configs in a single place?

We should also make sure that current linting is mapped to ruff

@tschm
Copy link
Contributor Author

tschm commented Nov 11, 2025

I am not sure it's a great idea to move ruff.toml in its entity to pyproject.toml. I think no linting should be in pyproject.toml

@tschm
Copy link
Contributor Author

tschm commented Nov 11, 2025

I remember why I like ruff.toml. It's a lot of material and usually it's very project independent. So you can copy it directly from a template. If it's integrated into pyproject it's harder

@@ -1,388 +1,391 @@
{
"cells": [
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we avoid linting settings that change the indentation in the notebooks?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isort messed with the notebook files. I recommend excluding the notebook files (from linting) for now

@tschm
Copy link
Contributor Author

tschm commented Nov 12, 2025

@fkiraly I have integrated ruff.toml into pyproject. Checked locally with "uvx ruff check ." still works. Excluded also the notebooks from listing (from now on...)

@tschm
Copy link
Contributor Author

tschm commented Nov 12, 2025

@fkiraly, please merge but without the notebooks if possible

@tschm
Copy link
Contributor Author

tschm commented Nov 14, 2025

@fkiraly please merge this before you change the notebooks again in the test notebooks request.

@fkiraly
Copy link
Collaborator

fkiraly commented Nov 14, 2025

@fkiraly please merge this before you change the notebooks again in the test notebooks request.

well, did not see this message.

The best practice to advertise sth like this is in PR2, you (a) include a description at the top, and (b) a line like "depends on #653 which should be merged first". Information on merge order desired should always be in the top descr.

Either way, what do we do now about the notebooks? Does not look like there is a merge conflict?

Copy link
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great PR!

Made some small changes:

  • removed pre-commit steps that are based on packages from small providers. This is to reduce the surface for security risks, we had supply chain attacks where small actions packages got attacked before.
  • moved the pre-commit job into main, and make it a gate for the other jobs, to ensure we do not run CI unless the files are linted.
  • applies linting only to changed files, or the job currently fails.

@fkiraly fkiraly merged commit e3036da into PyPortfolio:main Nov 14, 2025
24 checks passed
@tschm tschm deleted the editorconfig2 branch November 15, 2025 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maintenance Continuous integration, unit testing & package distribution

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants