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: code quality improvements #481

Conversation

cristi-iacob
Copy link
Contributor

Code quality improvements, accordingly to Codacy.

@rgetz
Copy link
Contributor

rgetz commented May 4, 2020

Looks like a good start. Thanks.

image

Can you have a look at:
image

seems to be an issue in both readdev.py and writedev.py

@rgetz rgetz requested a review from tfcollins May 4, 2020 14:05
@cristi-iacob cristi-iacob force-pushed the python-code-improvements branch from 6643615 to e430716 Compare May 4, 2020 14:18
@cristi-iacob
Copy link
Contributor Author

I have solved the import signal issue.

@rgetz
Copy link
Contributor

rgetz commented May 4, 2020

One last issue...

According to PEP8, if any return statement returns an expression, any return statements where no value is returned should explicitly state this as return None, and an explicit return statement should be present at the end of the function (if reachable)

This is flagged in iio_info.py in the _handle_context(): function.

@cristi-iacob cristi-iacob force-pushed the python-code-improvements branch 6 times, most recently from 5fca118 to d2f3b7b Compare May 5, 2020 15:10
@rgetz
Copy link
Contributor

rgetz commented May 5, 2020

I think you messed up the rebase - it includes some things that are already on master.

@rgetz
Copy link
Contributor

rgetz commented May 5, 2020

If you can focus on the examples - I already have a branch for the actual iio.py fixes. (at least from a linter perspective).

I will see if I can finish what I'm working on, and get back to that tomorrow.

@tfcollins
Copy link
Contributor

Need to ignore W0212. Otherwise the bindings would probably have to be rearchitected

@cristi-iacob cristi-iacob force-pushed the python-code-improvements branch from d2f3b7b to 767a8b8 Compare May 6, 2020 06:45
@cristi-iacob
Copy link
Contributor Author

I removed the commit with the modifications from iio.py and rebased the branch with the current master.

Made some modifications in order to improve the code quality, as suggested by Codacy.

Signed-off-by: Cristi Iacob <[email protected]>
Added some modifications for improving the code quality for the iio_writedev.py module, accordingly to Codacy.

Signed-off-by: Cristi Iacob <[email protected]>
Added some modifications for improving the code quality for the iio_info.py module, accordingly to Codacy.

Signed-off-by: Cristi Iacob <[email protected]>
@rgetz
Copy link
Contributor

rgetz commented May 7, 2020

RemarkLint has two issues:

  • [heading-increment] Heading levels should increment by one level at a time (line 15)
  • [fenced-code-flag] Missing code language flag (line 22) should be ```shell (like in README_BUILD.md)

@dNechita
Copy link
Contributor

Is there any more work needed to be done here? @cristi-iacob

@cristi-iacob
Copy link
Contributor Author

@dNechita no, it's all done.

@cristi-iacob
Copy link
Contributor Author

The issues from RemarkLink are resolved in #483.

@dNechita
Copy link
Contributor

@tfcollins is this ok to merge?

@rgetz
Copy link
Contributor

rgetz commented May 14, 2020

In discussion with @cristi-iacob ; he indicated that the python examples are being re-written in a more pythonic way; and this was close to being finished. (within a week or less).

It doesn't make sense to churn this with these fixes, and then replace things completely in a few days.

@dNechita
Copy link
Contributor

In discussion with @cristi-iacob ; he indicated that the python examples are being re-written in a more pythonic way; and this was close to being finished. (within a week or less).

It doesn't make sense to churn this with these fixes, and then replace things completely in a few days.

If this PR isn't required, @cristi-iacob please close it.

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.

4 participants