Skip to content

Conversation

@sfstar
Copy link
Owner

@sfstar sfstar commented Jan 29, 2025

to be merged after #278 is merged (also pr information needs expansion) changed has been merged.

TODO's

Introduce linting workflow item (to allow this to be enforced)
~~ Try to get this to run inside an checked out version of ha core dev so that the ruff settings can be inherited ~~ <-- implementation in seperate PR to improve reviewability.

Backport all non-formatting related changes from the remcom fork
possibly: prepare for pymodbus 3.9.0 by hauling over decoding logic (to be moved to other pr if this PR becomes massive) (This will be done in a seperate PR to improve reviewability)

julesxxl
julesxxl previously approved these changes Jan 29, 2025
Copy link
Collaborator

@julesxxl julesxxl left a comment

Choose a reason for hiding this comment

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

Haha. I think the .DS_Store and .idea appeared because of me...

@sfstar sfstar dismissed julesxxl’s stale review January 29, 2025 18:04

The merge-base changed after approval.

@sfstar sfstar force-pushed the feature/backport_the_changes_made_on_the_remcom_fork branch from 6f80dd7 to 945ab89 Compare January 29, 2025 18:20
@sfstar sfstar force-pushed the feature/backport_the_changes_made_on_the_remcom_fork branch from 945ab89 to 2335988 Compare January 29, 2025 18:21
@sfstar
Copy link
Owner Author

sfstar commented Jan 29, 2025

current state of PR is that almost all changes have been backported and some extra workflow steps for auto scanning ruff have been added. Last changes are some CI related changes that still need to be ported.

@sfstar
Copy link
Owner Author

sfstar commented Jan 30, 2025

After further consideration this PR will only focus on the PORT and necessary CI changes to mature this repository.
Including compatibility for version 3.9.0 would cause this PR to become to large to effectively review.

@sfstar
Copy link
Owner Author

sfstar commented Jan 30, 2025

@julesxxl and @remcom could you review the port of the fork at your convienence?
Some changes haven't been ported since they are included in #274.
Furthermore, some CI actions have been expanded upon or changed for compatibility.
Please let me know if there is anything missing :)

@sfstar sfstar changed the title [DRAFT] Feature/backport the changes made on the remcom fork Feature/backport the changes made on the remcom fork Jan 30, 2025
@remcom
Copy link

remcom commented Jan 31, 2025

@sfstar Nice! i will review it this weekend!

@julesxxl
Copy link
Collaborator

julesxxl commented Feb 1, 2025

Great!
So, on my side, I've checkout on this branch.
Ruff (as configured in HA pyproject.toml) complained against hub.py.
Mypy found 493 errors.
Pylint score is rated at 9.75/10 (less than 50 errors, most of them are "TODO" - that HA won't accept).
I'll try to fix all of that if you want, after a first check from you (manly the "TODO")

@sfstar
Copy link
Owner Author

sfstar commented Feb 1, 2025

Hello Jules,

Thanks for checking.
Could you share the commands used to get to these errors?
When running the pre-hook and/or ruff check at my end (either with local in repo .toml or ha ruff.toml) I only get some manifest complaints due to the integration having a loose version constraint, non-complaint document url etc.
Regarding the TODO's, I would suggest we leave them for now and clean them up when changing logic in upcoming PR's that will (among others) change over the decoding logic.

@julesxxl
Copy link
Collaborator

julesxxl commented Feb 1, 2025

Hello Jules,

Thanks for checking. Could you share the commands used to get to these errors? When running the pre-hook and/or ruff check at my end (either with local in repo .toml or ha ruff.toml) I only get some manifest complaints due to the integration having a loose version constraint, non-complaint document url etc. Regarding the TODO's, I would suggest we leave them for now and clean them up when changing logic in upcoming PR's that will (among others) change over the decoding logic.

Firstly, I checked their .pre-commit-config.yaml for the commands. Then, I copied-pasted their mypy.ini (don't really know why they don't put it in the pyproject.tom).
So, for ruff, pylint and mypy, from the root folder:

mypy custom_components
pylint --ignore-missing-annotations=y --score=y custom_components
ruff check --fix custom_components
ruff format custom_components

They also use other tools. An interesting one for us: codespell. As for the checking of the yaml, json, the pretty format, the requirements generation, etc., we don't mind.

@remcom
Copy link

remcom commented Feb 2, 2025

@sfstar i tested the changes and all still works as expected. Only one small thing in custom_components/victron/manifest.json you need to put the version to "v0.0.0". Otherwise the release script wont update the version

@sfstar
Copy link
Owner Author

sfstar commented Feb 2, 2025

Thanks for checking jules and remcom,

I've changed the manifest.json to have the version number as v0.0.0

@julesxxl, thanks for providing the commands.
Based on the output it seems that most findings are related to TODO's and the const.py enum passthrough.
#274 adds all registers that were added by victron since the integrations inception.
I believe updating this PR to resolve all const.py issues would resolve in a lot of merge conflicts.
Therefore, I would like to suggest that we merge this PR, then #274, before picking up the issues reported by pylint/mypy.

If you would like to make these changes jules, then let me know (I will initially be focussing on pymodbus v3.9.0 support) after these PR's. (So that we can parrallel changes without having to worry about to many conflict ;) )

@julesxxl
Copy link
Collaborator

julesxxl commented Feb 2, 2025

@sfstar: ok, we'll see the linting/typing after these PR.

@sfstar sfstar merged commit ef427cf into main Feb 3, 2025
3 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.

4 participants