- Notifications
You must be signed in to change notification settings - Fork123
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Python examples rework#27
base:master
Are you sure you want to change the base?
Conversation
4780e06 to0d1e765Compareb3779ae to623df66CompareNavio: Navio+ modulesNavio2: Navio2 modulesCommon: common modulesAdd 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
623df66 to66d83feCompareMake 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 checkAdd 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 folderDelete useless import util
Add Navio+ entry and add auto choice shield
Add Navio+ entry and auto choice shield
Add root checkAdd Navio+ entry and add shield auto choice
66d83fe to323675fCompare
kobylyanskiy left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good job!
Nice to hear! But could you please provide any details or close the PR as is. And, if you approve PR, could you merge it? |
kobylyanskiy commentedApr 8, 2024
I've already approved, but cannot merge. Sorry :( |
Could you mention someone, who can?@staroselskii for example |
kobylyanskiy commentedApr 8, 2024
@vldpro could you please help us here? |
vldpro commentedApr 8, 2024
@IgorAnohin Could you reformat the code with 4-space indentation? I can't really read this. |
If I use |
vldpro commentedApr 8, 2024
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 commentedApr 8, 2024
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. |
yashin-alexander left a comment
There was a problem hiding this 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 commentedApr 8, 2024
@AlexanderDranitsa could you please help us finish this? |
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. |
dskorykh left a comment
There was a problem hiding this 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): |
There was a problem hiding this comment.
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
| defsetColor(self,color): | |
| defset_color(self,color): |
| return value[:-1] | ||
| except IndexError: | ||
| print("Channel number too large") | ||
| exit(1) |
There was a problem hiding this comment.
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
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 commentedApr 9, 2024
This is kind of a joke, but seriously, we need to get this done. Let's find someone who can make things happen! |
Hello!
I reworked python examples. What do you think about this?
Could you review it?