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

Expression Code: Refactor Multiple Lines of Code #188

Merged
merged 3 commits into from Oct 12, 2021

Conversation

yezz123
Copy link
Contributor

@yezz123 yezz123 commented Oct 6, 2021

Description

I just fix some parts of the code refactoring them, Also fix some code issues related to the coming version Py3.6<.
For example :

  • Replace if statement with if expression, Simplify if expression by using or
  • Merge nested if conditions
  • Simplify conditional into the return statement
  • Inline variable that is immediately returned
  • Replace if statement with if expression, Simplify boolean if the expression
  • Simplify comparison to boolean, Simplify logical expression using De Morgan identities
  • Replace if statement with if the expression
  • Merge nested if conditions, Replace unneeded comprehension with the generator.
  • Invert any/all to simplify comparisons
  • Convert for loop into list comprehension, Inline variable that is immediately returned, Remove unnecessary else after guard condition

Type of change

  • This change requires a documentation update
  • Refactoring Code

How Has This Been Tested?

I run the linter to lint the project before the push, also after installing all requirements, I try some commands:

make test
make test_unit

Checklist:

  • My code follows the style guidelines of this project (no GitHub actions complaints! run make lint before
    committing!)
  • I have commented my code, pydocstyle, and darglint are happy, docstrings are in google docstring format, and all
    docstrings include a summary, args, returns, and raises fields (even if N/A)
  • I have added tests that prove my fix is effective or that my feature works, if adding "functional" tests, please
    note if there are any considerations for the vrnetlab setup
  • New and existing unit tests pass locally with my changes

@codecov
Copy link

codecov bot commented Oct 7, 2021

Codecov Report

Merging #188 (e0170a5) into develop (7af5387) will decrease coverage by 0.08%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #188      +/-   ##
===========================================
- Coverage    91.04%   90.95%   -0.09%     
===========================================
  Files           58       58              
  Lines         3304     3274      -30     
===========================================
- Hits          3008     2978      -30     
  Misses         296      296              
Impacted Files Coverage Δ
scrapli/channel/async_channel.py 95.65% <100.00%> (-0.02%) ⬇️
scrapli/channel/sync_channel.py 97.27% <100.00%> (-0.02%) ⬇️
scrapli/driver/base/async_driver.py 100.00% <100.00%> (ø)
scrapli/driver/base/sync_driver.py 100.00% <100.00%> (ø)
scrapli/driver/generic/async_driver.py 98.14% <100.00%> (ø)
scrapli/driver/generic/base_driver.py 100.00% <100.00%> (ø)
scrapli/driver/generic/sync_driver.py 98.14% <100.00%> (ø)
scrapli/factory.py 95.83% <100.00%> (-0.03%) ⬇️
scrapli/helper.py 100.00% <100.00%> (ø)
scrapli/logging.py 83.49% <100.00%> (-0.62%) ⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7af5387...e0170a5. Read the comment docs.

@carlmontanari
Copy link
Owner

Hey @yezz123 thanks for creating this!

Would you mind rebasing from develop branch? (Sorry, this is my fault, ive been meaning to set develop as the default branch and keep forgetting...). I took a quick peak and looks like a lot of sound fixes/improvements to me. I'll probably take a closer look this weekend and then may have some questions/comments -- will keep you posted!

Thanks again!

Carl

@yezz123 yezz123 changed the base branch from master to develop October 7, 2021 00:40
@yezz123
Copy link
Contributor Author

yezz123 commented Oct 7, 2021

Hey @yezz123 thanks for creating this!

Would you mind rebasing from develop branch? (Sorry, this is my fault, I've been meaning to set develop as the default branch and keep forgetting...). I took a quick peek and looks like a lot of sound fixes/improvements to me. I'll probably take a closer look this weekend and they may have some questions/comments -- will keep you posted!

Thanks again!

Carl

Hello Carl,

I just rebase to the develop branch, this PR only help to enhance & Refactor the Code to be more readable by humans and easy to interact for the machine, I will try to fix the Coverage issue, then by the weekend we can discuss this PR. 🚀

Copy link
Owner

@carlmontanari carlmontanari left a comment

Choose a reason for hiding this comment

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

Hey @yezz123 finally got to taking a peak! Good stuff in here! Dropped a few minor comments in the review if you could take a peak and let me know what you think. The only sorta big thing is the mypy complaints... I put a proposed "fix" in there but would love to see what you think!

Thanks again for your work on this!

Carl

scrapli/driver/generic/sync_driver.py Show resolved Hide resolved
scrapli/driver/base/sync_driver.py Show resolved Hide resolved
scrapli/channel/sync_channel.py Show resolved Hide resolved
scrapli/response.py Outdated Show resolved Hide resolved
@yezz123
Copy link
Contributor Author

yezz123 commented Oct 11, 2021

Hey @carlmontanari, sorry for the late responding, I have some other work that why I add this PR to bookmarks, now I just fix all that you request to change and I guess now its look good 👌🏻

@yezz123
Copy link
Contributor Author

yezz123 commented Oct 11, 2021

I guess the issue come from pydocstyle let me check how i can fix it 👌🏻

@yezz123
Copy link
Contributor Author

yezz123 commented Oct 11, 2021

I just Fix it ✨

Copy link
Owner

@carlmontanari carlmontanari left a comment

Choose a reason for hiding this comment

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

Looks like doc test fails since the commit you rebased off of had an error (not your fault of course!), and there is one darglint complaint that ill fix since it can be a bit of a pain :)

Thanks for working with me on this one -- some nice improvements in here!

Carl

@carlmontanari carlmontanari merged commit 3afb381 into carlmontanari:develop Oct 12, 2021
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.

None yet

2 participants