Fix multiple associations and removals of networks

[sveinse]

This PR fixes the issue of not being able to call associate_network() and remove_network() multiple times. See #562.

Fixes #562

[acolomb]

Nice. Mainly nit-picks, and I do welcome to use the existing .has_network() method. I don't quite think automatically re-associating to a different network is appropriate. Associating twice without removing from the first network will more likely point toward a programming error rather than a conscious, common workflow.

[codecov]

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment

Thanks for integrating Codecov - We've got you covered ☂️

[acolomb]

I meant something like this instead, with a single del code-path. I find that easier to reason about:

        if callback is not None:
            self.subscribers[can_id].remove(callback)
        if not self.subscribers[can_id] or callback is None:
            del self.subscribers[can_id]

[sveinse]

Yes, two lines shorter.

[acolomb]

These seem to be not needed.

Suggested change

	import canopen.network
	import canopen.objectdictionary

[sveinse]

They are. Some linters, like pyright, is complaining when they are lacking. The reason is that they are not referenced in the __init__.py exports, so they need to be explicitly imported.

[acolomb]

I understand. How come the code works without them though?

[acolomb]

I'm not really fond of having comments just for the sake of having a comment. None of our test files so far have a module-level docstring. There is no generated API documentation for them. And import unittest says just as much as this comment, while the enumeration of classes is redundant and bound to get out of sync sometime.

In essence, don't feel obliged to write docstrings that add no value. Or try to come up with a really good one that does so.

What IS the common denominator for stuff that will be grouped in this test file? So far it is only network association, what else do you have in mind?

Suggested change

"""Unit tests for the RemoteNode and LocalNode classes."""

[sveinse]

What IS the common denominator for stuff that will be grouped in this test file? So far it is only network association, what else do you have in mind?

Eventually we'll have more unit tests of the methods in these two classes, no?

[acolomb]

Maybe, that's why I'm asking if you have any concrete ideas what will be added?

[acolomb]

I took the liberty to apply my suggested changes. Should be uncontroversial I hope. Please indicate whether you are happy with the last commit's logic "simplification", it might seem simpler only to me :-)

[sveinse]

Comments updated. No objections.

You and I have very different coding styles my friend 😎 While in many projects the commit is rejected for not having docstrings, here its removed because its not informative enough 🎉 It's fine, I just find it amusing. One certainly needs to learn every project's individual styles, that's for sure. Even the undocumented ones :D

PS! I don't know, but I don't have push authorization no more since the project move. Was that deliberate?

[acolomb]

Yes, I know my stance on coding style is not average. And I'm happy that you're open to adjusting. It's mostly stuff where my experience tells me that exceptions from any rule might be better than following it blindly. There should always be room for well-reasoned deviation.

To the question at hand: I really hate comments stating the obvious that give no further information than what the (well-written) code already tells. It's just twice the reading effort, and only makes sense if the docstrings are needed for auto-generated docs. This is different in the context of unit tests, not the actual library classes. And if we wanted to be perfect, there would be many many test functions with "Missing docstring in public method" messages to tackle.

PS! I don't know, but I don't have push authorization no more since the project move. Was that deliberate?

Not that I know of. The move was done by Christian, I haven't adjusted any permissions since. What exactly can you no longer do?

Actually, I did change one thing: Activate branch protections for the master branch. That was needed to differentiate the required / optional checks. And in the process I blocked direct pushes to master, they need to go through a PR with approval (except for organization admins who may bypass that).

[sveinse]

Yes, I know my stance on coding style is not average. And I'm happy that you're open to adjusting. It's mostly stuff where my experience tells me that exceptions from any rule might be better than following it blindly. There should always be room for well-reasoned deviation.

Well, in practice I don't have much choice, do I? Going into a PR where someone adamantly disagrees with the style is not a good way to create a cooperative environment, right. We have far harder challenges ahead of us with respect to asyncio than to squabble over formatting styles.

PS! I don't know, but I don't have push authorization no more since the project move. Was that deliberate?

Not that I know of. The move was done by Christian, I haven't adjusted any permissions since. What exactly can you no longer do?

I can't merge this branch, even when it's approved.

[acolomb]

I can't merge this branch, even when it's approved.

Hope it works now. My intention was to disallow direct pushes, but merging pull requests should still be allowed for people with write access (which you do have). I disabled the "Restrict who can push to matching branches" setting again, hope that has the desired effect.

[sveinse]

I have access now. Thanks!

Since we're into discussions about merging: What is your preferred approach to merge? Is squash and merge the way to go? -- I personally tend to prefer that with public PRs, since there often will be a bit back and forth, like this one, where there will be many minor tweak commits.

[acolomb]

Yes, squash and merge should do it. Unless we have special circumstances with a perfectly hand-crafted commit history. Which is pretty rare.

[sveinse]

Merged! Thank you for the review @acolomb

[acolomb]

Great! Thank you very much for your continued involvement and you patience with reviews :-)

[acolomb]

One more thing about the removed imports, forgot to write it down: The canopen.objectdictionary.ObjectDictionary is in fact available as just canopen.ObjectDictionary. Which we should have used.

The same doesn't apply to the _UNINITIALIZED_NETWORK thing. Which should be non-public API anyway.

As long as the tests work, I don't mind much about it though. It's only unit test code 😉