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

Optimise codebase according to latest pythonic code style #130

Merged
merged 3 commits into from
May 13, 2022

Conversation

Agent-Hellboy
Copy link
Contributor

@Agent-Hellboy Agent-Hellboy commented May 11, 2022

Consider this as an WIP PR, I will try to cover more methods as I need to solve some circular dependencies, I am thinking of using mypy for type checking, I can see that unittest are missing for base.py, maybe i will add those as well. Or if you want i can create a new issue named increase code coverage

I have made one extra change of converting all instances of string formatting using .format() to f string.

@codecov-commenter
Copy link

Codecov Report

Merging #130 (d7a18b6) into master (4ad277e) will increase coverage by 0.05%.
The diff coverage is 85.89%.

@@            Coverage Diff             @@
##           master     #130      +/-   ##
==========================================
+ Coverage   83.13%   83.19%   +0.05%     
==========================================
  Files          11       11              
  Lines        1405     1410       +5     
  Branches      249      250       +1     
==========================================
+ Hits         1168     1173       +5     
  Misses        159      159              
  Partials       78       78              
Impacted Files Coverage Δ
beautifultable/meta.py 77.27% <80.00%> (+1.08%) ⬆️
beautifultable/helpers.py 78.91% <83.33%> (+0.04%) ⬆️
beautifultable/beautifultable.py 83.29% <86.66%> (+0.07%) ⬆️
beautifultable/ansi.py 71.60% <100.00%> (ø)
beautifultable/base.py 85.05% <100.00%> (ø)
beautifultable/styles.py 100.00% <100.00%> (ø)
beautifultable/utils.py 76.19% <100.00%> (+0.58%) ⬆️

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 eec3e63...d7a18b6. Read the comment docs.

@pri22296
Copy link
Owner

I agree with everything you proposed. I would prefer if we can have the f-string change in a separate PR with it's own issue. same with the other changes like including tests, integration with mypy. Will allow smaller changes to be checked in without getting blocked by unrelated blockers.

For type annotations, It would be easier to have a single issue tracking other child issues, wherein you can have smaller PR's for maybe each module at a time. Would make reviews easier.

- Use f-string for string formatting
- Remove explicit inheritance from python object
- use pythonic APIs like all,any.. wherever possible
@Agent-Hellboy
Copy link
Contributor Author

Agent-Hellboy commented May 12, 2022

fixes

@Agent-Hellboy Agent-Hellboy changed the title Add type annotations Optimise codebase according to latest pythonic code style May 12, 2022
@pri22296
Copy link
Owner

pri22296 commented May 13, 2022

@Agent-Hellboy Is this PR still WIP? If yes, can you then add a [WIP] tag before PR's title. If no, let me know and I'll approve this.

@Agent-Hellboy
Copy link
Contributor Author

This is final

@pri22296 pri22296 merged commit 5bce31d into pri22296:master May 13, 2022
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.

3 participants