Skip to content

repl config path #1359

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.

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

Merged
merged 5 commits into from
Feb 19, 2023
Merged

repl config path #1359

merged 5 commits into from
Feb 19, 2023

Conversation

alexrudd2
Copy link
Collaborator

@alexrudd2 alexrudd2 commented Feb 19, 2023

mypy is not happy with the option modbus_config being used for both a Path and a dictionary (possibly) loaded from that path. This fixes another 9 errors.

Since this is technically an interface change, I've kept it separate from my other type fixes. Another option to to use e.g. modbus_config_dict but that seemed verbose.

if modbus_config:
    with open(modbus_config) as my_file:  # pylint: disable=unspecified-encoding
        modbus_config_dict = json.load(my_file)

@dhoomakethu

@alexrudd2 alexrudd2 changed the base branch from dev to types February 19, 2023 01:13
Base automatically changed from types to dev February 19, 2023 07:45
Copy link
Collaborator

@janiversen janiversen left a comment

Choose a reason for hiding this comment

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

LGTM, but please fix “ # pylint: disable=unspecified-encoding” while you are at in (copied from the description of this PR.

I am also very unhappy about the SQL-stubs, its an extra requirement that really are not needed.

@dhoomakethu
Copy link
Contributor

LGTM. Thanks

@janiversen
Copy link
Collaborator

Added commit to:

  • add mypy==1.0.1 (but currently do not activate it in CI or check_ci)
  • Remove SQLAlchemyStubs.

If a library do not contain type hints, we should not add extra requirements just to cover that. We use our own version of async-pyserial that too comes with a level of type hints.

The sqlalchemystub slipped through my review, hence correcting it.

@janiversen
Copy link
Collaborator

Added commit to solve black/pylint.

@janiversen
Copy link
Collaborator

Added commit with more comments in mixin, to explain the use of execute().

Mypy needs to be silenced for the execute function (not all the others).

@alexrudd2 I will leave it up to you to correct or merge this, since I have corrected it (You have 2 approvals).

alexrudd2 and others added 5 commits February 19, 2023 11:53

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Co-authored-by: jan iversen <jancasacondor@gmail.com>
@alexrudd2
Copy link
Collaborator Author

Screenshot 2023-02-19 at 12 02 06 PM

It seems there is an automatic rule to change PR targets which caused some confusion. I intended this to be merged into #1356 Sorry

Anyways, I rebased it to squash the lint commits.

@alexrudd2
Copy link
Collaborator Author

This is totally unrelated to #1357 since I do not yet have a solution to it, so I removed that closing keyword.

@alexrudd2
Copy link
Collaborator Author

@alexrudd2 I will leave it up to you to correct or merge this, since I have corrected it (You have 2 approvals).

I think this PR is now ready. However, dev is a protected branch, and I'm not authorized to merge into it.

@janiversen
Copy link
Collaborator

I thought you were, I will take a look on that later. The protection should only require an approval, which you have.

@janiversen janiversen merged commit 53aa1ca into dev Feb 19, 2023
@janiversen janiversen deleted the repl-config-path branch February 19, 2023 18:24
alexrudd2 added a commit to alexrudd2/pymodbus that referenced this pull request Feb 21, 2023
* more mypy.

Co-authored-by: jan iversen <jancasacondor@gmail.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants