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

Python examples rework #27

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

IgorAnohin
Copy link
Contributor

@IgorAnohin IgorAnohin commented Nov 17, 2017

Hello!
I reworked python examples. What do you think about this?
Could you review it?

@IgorAnohin IgorAnohin force-pushed the pr-python-examples-rework branch 2 times, most recently from 4780e06 to 0d1e765 Compare December 21, 2017 12:49
@IgorAnohin IgorAnohin force-pushed the pr-python-examples-rework branch 5 times, most recently from b3779ae to 623df66 Compare January 9, 2018 12:10
Igor Anokhin added 4 commits January 10, 2018 12:41
Navio: Navio+ modules
Navio2: Navio2 modules
Common: common modules
Add all modules in __init__ files in each folder
Add function get_navio_version, which return Navio version
Make new module and add this module in __init__ file
Make ADC module for Navio+ and add this module in __init__ file
Igor Anokhin added 10 commits January 10, 2018 12:49
Make new RCInput module for Navio+ in add this module in __init__ file
Make RCOutput module for Navio+ and add this in __init__ file
Print error message if channel too large
Add root check
Add Navio+ entry and auto choice shield
Add Navio+ entry and auto choice shield
All modules for this example now in folders Common
All modules for this example now in Common folder
Delete useless import util
Add Navio+ entry and add auto choice shield
Add Navio+ entry and auto choice shield
Add root check
Add Navio+ entry and add shield auto choice
Copy link

@kobylyanskiy kobylyanskiy left a comment

Choose a reason for hiding this comment

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

Good job!

@IgorAnohin
Copy link
Contributor Author

Good job!

Nice to hear! But could you please provide any details or close the PR as is.
I've been waiting so long for any review...

And, if you approve PR, could you merge it?

@kobylyanskiy
Copy link

I've already approved, but cannot merge. Sorry :(

@IgorAnohin
Copy link
Contributor Author

I've already approved, but cannot merge. Sorry :(

Could you mention someone, who can? @staroselskii for example

@kobylyanskiy
Copy link

@vldpro could you please help us here?

@vldpro
Copy link

vldpro commented Apr 8, 2024

@IgorAnohin Could you reformat the code with 4-space indentation? I can't really read this.

@IgorAnohin
Copy link
Contributor Author

@IgorAnohin Could you reformat the code with 4-space indentation? I can't really read this.

If I use black, will it be OK?

@vldpro
Copy link

vldpro commented Apr 8, 2024

@IgorAnohin Could you reformat the code with 4-space indentation? I can't really read this.

If I use black, will it be OK?

I hope it won't take another 7 years.

@IgorAnohin
Copy link
Contributor Author

@IgorAnohin Could you reformat the code with 4-space indentation? I can't really read this.

If I use black, will it be OK?

I hope it won't take another 7 years.

I've already pushed. You can check the new code using "Files Changed".

Unfortunately, to make a real improvement in the readability of this PR, 1 more PR is required. Only with “black” changes.

But I don't have enough time for that. So the current version is all I can do.

@vldpro
Copy link

vldpro commented Apr 8, 2024

@IgorAnohin Could you reformat the code with 4-space indentation? I can't really read this.

If I use black, will it be OK?

I hope it won't take another 7 years.

I've already pushed. You can check the new code using "Files Changed".

Unfortunately, to make a real improvement in the readability of this PR, 1 more PR is required. Only with “black” changes.

But I don't have enough time for that. So the current version is all I can do.

It's already a way better! I think we need to get this work done. These examples have been waiting for 7 years to be merged! I think we need someone who has real power to merge it.

Copy link

@yashin-alexander yashin-alexander 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 can you please adapt these examples for python3 too?

@kobylyanskiy
Copy link

@AlexanderDranitsa could you please help us finish this?

@AlexanderDranitsa
Copy link
Collaborator

Igor, I'm excited to see this code is getting better and better. I'm not sure if it is good enough, though. At least, I agree with @yashin-alexander, you should probably go for Python 3 now.
Anyway, I can't merge this too, you should ask someone else!

Copy link

@dskorykh dskorykh left a comment

Choose a reason for hiding this comment

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

Good job, @IgorAnohin! Needs some improvements, but they are not critical. As @kobylyanskiy mentioned, there are some conflicts that should be resolved

self.ledG.write(OFF)
self.ledB.write(OFF)

def setColor(self, color):
Copy link

Choose a reason for hiding this comment

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

Please, bring the code to PEP8 compliance. I see a lot of camelCase naming here

Suggested change
def setColor(self, color):
def set_color(self, color):

return value[:-1]
except IndexError:
print("Channel number too large")
exit(1)
Copy link

Choose a reason for hiding this comment

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

It's a good practice to throw exception on higher level instead of exiting inside utility method

@kobylyanskiy
Copy link

kobylyanskiy commented Apr 9, 2024

It's truly disappointing that the review of this feature has taken nearly 7 years, causing significant slowdown for everyone involved.

@dskorykh, I appreciate your attention to this, but we need to find someone with the real power to finish our quest.

@kobylyanskiy
Copy link

This is kind of a joke, but seriously, we need to get this done. Let's find someone who can make things happen!

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.

6 participants