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.

Sign up for GitHub

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

Jump to bottom

Fix route map generation for classes using @property #140

Merged
merged 8 commits into from
Oct 27, 2020
Merged

Fix route map generation for classes using @property #140

merged 8 commits into from
Oct 27, 2020

Conversation

fa1k3n
Copy link
Contributor

@fa1k3n fa1k3n commented Oct 21, 2020

Instead of iterating over the objects dir attribute the on and after decorators saves the function name in list. This list is then iterated to build rout map.

Testcase added to verify the fix

@ghost
Copy link

ghost commented Oct 21, 2020 •
edited by ghost
Loading

DeepCode's analysis on #085bb4 found:

  • ℹ️ 1 minor issue. 👇

Top issues

Description Example fixes
Undefined variable 'pytest' Occurrences: 🔧 Example fixes

👉 View analysis in DeepCode’s Dashboard | Configure the bot

@fa1k3n
Copy link
Contributor Author

fa1k3n commented Oct 21, 2020

I fixade the issues but how do I rerun the tests?

tropxy
tropxy suggested changes Oct 21, 2020
View reviewed changes
tropxy
tropxy previously approved these changes Oct 21, 2020
View reviewed changes
Copy link
Collaborator

@tropxy tropxy left a comment

Choose a reason for hiding this comment

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

Cool! LGTM ;)

OrangeTux
OrangeTux suggested changes Oct 22, 2020
View reviewed changes
OrangeTux
OrangeTux suggested changes Oct 22, 2020
View reviewed changes
@OrangeTux OrangeTux changed the title added fix for issue 137 Fix route map generation for classes using @property Oct 22, 2020
tropxy
tropxy suggested changes Oct 26, 2020
View reviewed changes
Copy link
Collaborator

@tropxy tropxy left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Was there a particular reason for the removal of the docs?

@fa1k3n
Copy link
Contributor Author

fa1k3n commented Oct 26, 2020 via email

@fa1k3n
Copy link
Contributor Author

fa1k3n commented Oct 26, 2020

Was there a particular reason for the removal of the docs?

sorry, It is now fixed! What happened was that my computer (an apple) in its infinite visdom synced the files to the cloud and then deleted them locally without my knowing it..... gotta love them smart putors..........

OrangeTux
OrangeTux previously approved these changes Oct 26, 2020
View reviewed changes
Copy link
Contributor

@OrangeTux OrangeTux left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for your effort!

tropxy
tropxy previously approved these changes Oct 26, 2020
View reviewed changes
Copy link
Collaborator

@tropxy tropxy left a comment

Choose a reason for hiding this comment

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

Thanks for this!

@OrangeTux
Copy link
Contributor

I'm not sure why CircleCI doesn't want to run the builds.

@fa1k3n fa1k3n dismissed stale reviews from tropxy and OrangeTux via 771659e October 26, 2020 18:28
@fa1k3n
Copy link
Contributor Author

fa1k3n commented Oct 26, 2020

I have fixed the Ci issues with newlines etc so now it passes

tropxy
tropxy approved these changes Oct 27, 2020
View reviewed changes
Copy link
Collaborator

@tropxy tropxy left a comment

Choose a reason for hiding this comment

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

Again, thanks and nice job ;)

@fa1k3n
Copy link
Contributor Author

fa1k3n commented Oct 27, 2020

Again, thanks and nice job ;)

Thanks and no problems, was a learning experience. Will you merge this or are you waiting for me to do something? How long before the changes appears in PIP?

@OrangeTux OrangeTux merged commit 11904af into mobilityhouse:master Oct 27, 2020
@OrangeTux
Copy link
Contributor

OrangeTux commented Oct 27, 2020 •
edited
Loading

As soon as possible. I opened a PR #146 to release 0.8.0 . If that is merged I'll publish the version 0.8.0.

@OrangeTux
Copy link
Contributor

Version 0.8.0 has been released.

ajmirsky pushed a commit to ajmirsky/ocpp that referenced this pull request Nov 26, 2024
@fa1k3n @OrangeTux
Fix route map generation for classes using @Property (mobilityhouse#140)
725dc9a 
For subclasses of `ChargePoint` using `@property` generating the route map could have unintended
side effects. The function `ocpp.routing.create_route_map()` iterates over all attributes of the subclass, thus
executing the method decorated with `@property`. 

This commit addressed that issue. 

Fixed: mobilityhouse#137 

Co-authored-by: Auke Willem Oosterhoff <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tropxy tropxy tropxy approved these changes

@OrangeTux OrangeTux OrangeTux left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fa1k3n @OrangeTux @tropxy