Skip to content
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

EventID has no nodeId but it is being used. #57

Closed
Poikilos opened this issue Jan 10, 2025 · 4 comments · Fixed by #60
Closed

EventID has no nodeId but it is being used. #57

Poikilos opened this issue Jan 10, 2025 · 4 comments · Fixed by #60

Comments

@Poikilos
Copy link
Contributor

Poikilos commented Jan 10, 2025

This looks like maybe a type-o carried over from reusing code from the NodeID constructor. The EventID constructor has this code and it appears incorrect since EventID has no nodeId. What should this look like?

        elif isinstance(data, EventID):
            self.eventId = data.nodeId

probably change to self.eventId = data.eventId

but I think EventID and NodeID would be less confusing to read and use if we change EventID's eventID and NodeID's nodeID to just self.value, since the type is int not the parent class type as the name implies in other parts of the code and typical conventions. Let me know your thoughts.

Poikilos added a commit to Hierosoft/python-openlcb that referenced this issue Jan 10, 2025
@bobjacobsen
Copy link
Owner

@Poikilos A good catch! Looks like a copy&paste error on my part. Do you have a branch with this fixed? If not, I can make a quick PR for it.

@Poikilos
Copy link
Contributor Author

Ok, I was in a game jam last week but it looks good. I can make a PR with the change to "value" which can help indicate the type is int after the PR is merged or I can create a separate PR.

@bobjacobsen
Copy link
Owner

I merged this PR. If you could PR an update to "value", that would be great.

@Poikilos
Copy link
Contributor Author

Yes, I am working on that and UTF-8.

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 a pull request may close this issue.

2 participants