Skip to content

Conversation

@as3ii
Copy link
Contributor

@as3ii as3ii commented Nov 4, 2025

If this ruff settings is fine for you, I'll start fixing all the issues reported by ruff, adding them as new commits in this PR

Closes #1020

@DJ2LS DJ2LS marked this pull request as ready for review November 8, 2025 19:58
@DJ2LS
Copy link
Owner

DJ2LS commented Nov 8, 2025

Sorry for my delayed answers, I'm actually preparing for my next exam....

The PR is looking good to me, if I interpreted it correctly, we will have a failed runner as soon as a linting rule matches, right?

I would suggest adding a short tutorial, maybe a contributor reader as suggested by GitHub, for adding lint8ng instructions. Say, a lint8ng rule can be executed per error code. What do you think?

@as3ii
Copy link
Contributor Author

as3ii commented Nov 11, 2025

The PR is looking good to me, if I interpreted it correctly, we will have a failed runner as soon as a linting rule matches, right?

yes, correct

I would suggest adding a short tutorial, maybe a contributor reader as suggested by GitHub, for adding lint8ng instructions. Say, a lint8ng rule can be executed per error code. What do you think?

Where should I put the instructions? In a "Contributing" section in the README or in a separate CONTRIBUTING.md?

@DJ2LS
Copy link
Owner

DJ2LS commented Nov 11, 2025 via email

@DJ2LS
Copy link
Owner

DJ2LS commented Nov 12, 2025

Regarding fixing of PEP errors reported by ruff - I'd like to review the changes, so what about merging this one into develop, and opening separate PRs for each fix, e.g. "Fixing PEP F541". I appreciate the work on linting, but I'm still carefully with auto fixes.

@as3ii as3ii changed the title Switched to ruff for linting/formatting, added ruff CI Switched to ruff for linting/formatting, added ruff CI, added CONTIRBUTING.md Nov 12, 2025
@as3ii as3ii marked this pull request as draft November 12, 2025 12:57
@as3ii
Copy link
Contributor Author

as3ii commented Nov 12, 2025

@DJ2LS I started manually fixing E4 rules, and now i have a question:

freedata_server/server.py, lines 3-5: why? Is there a way to test better alternatives without having apple hardware? (does the removal of those lines break some CI workflows?) Maybe using relative path imports like from . import ABC? (Or from ..freedata_server import ABC for the tests)

Editing sys.path inside py files is not a good practice an should be avoided

Same for test/test_config.py, test/test_data_frame_factory.py, test/test_data_type_handler.py and others

PS. ruff's auto-fixes covers only simple and safe cases (testing and manual review of the changes must be done in any case)

@DJ2LS
Copy link
Owner

DJ2LS commented Nov 12, 2025

Regarding lines 3-5, not sure anymore tbh, but pip is not that critical at the moment. Maybe there is a better way combined with new project toll file

@DJ2LS
Copy link
Owner

DJ2LS commented Nov 12, 2025

Feel free using auto fixes, my intention is just keeping reviewing easier by not being overload by too many changes 🙂

@DJ2LS
Copy link
Owner

DJ2LS commented Nov 12, 2025

Regarding the sys path stuff, if you have an idea how to fix it, then feel free doing this, I won't have much time at the moment. I'm always open to learn something new and improving my skills, so I'm looking forward to your suggestions 👍

@as3ii
Copy link
Contributor Author

as3ii commented Nov 12, 2025

Regarding the sys path stuff, if you have an idea how to fix it, then feel free doing this, I won't have much time at the moment. I'm always open to learn something new and improving my skills, so I'm looking forward to your suggestions 👍

the main issue here is avoiding regressions, I don't know what was the original issue and how to replicate it to test if the alternative solution works as expected. Applying an alternative solution an waiting for any complaints is a possibility that I don't know if you want to take

@DJ2LS
Copy link
Owner

DJ2LS commented Nov 13, 2025

Regarding the sys path stuff, if you have an idea how to fix it, then feel free doing this, I won't have much time at the moment. I'm always open to learn something new and improving my skills, so I'm looking forward to your suggestions 👍

the main issue here is avoiding regressions, I don't know what was the original issue and how to replicate it to test if the alternative solution works as expected. Applying an alternative solution an waiting for any complaints is a possibility that I don't know if you want to take

Sounds good to me, because the pip package isn't "officially" released. I did this to register and save the package name. So in my opinion we can remove this part of code and optimize the pip build process, waiting for feedback then if problems occur.

@DJ2LS DJ2LS marked this pull request as ready for review November 13, 2025 07:29
@DJ2LS DJ2LS marked this pull request as draft November 13, 2025 07:29
@DJ2LS DJ2LS added this to FreeDATA Nov 13, 2025
@DJ2LS DJ2LS added this to the 1.0.0 milestone Nov 13, 2025
@DJ2LS DJ2LS moved this to In progress in FreeDATA Nov 13, 2025
@DJ2LS DJ2LS added the enhancement New feature or request label Nov 13, 2025
This applies mostly the same rules that `black` used.

This commit should not have introduced any logical or functional
changes, only fixing style consistency
See: https://peps.python.org/pep-0008/#imports

Here absolute imports have been used to avoid the error
`ImportError: attempted relative import with no known parent package`.
@as3ii as3ii marked this pull request as ready for review November 13, 2025 22:49
@as3ii
Copy link
Contributor Author

as3ii commented Nov 13, 2025

Sadly this is quite a big PR, mainly due to 66b2c6b (first run of the formatter), luckily that commit don't need any in-depth review or testing. The main commit I'm in doubt about is 90a1e7c.
I've done some fast testing on these changes, but at the moment I don't have my shack at hand and a more painstaking test with flrig/fldigi/hamlib is needed

@DJ2LS
Copy link
Owner

DJ2LS commented Nov 15, 2025

hard to test everything - suggestion: if tests are passing and functionality is given without errors on a "real world testing" we can approve this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

Status: In progress

Development

Successfully merging this pull request may close these issues.

2 participants