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

Major rework: more pythonic API, code rework #38

Merged
merged 26 commits into from
May 8, 2020
Merged

Major rework: more pythonic API, code rework #38

merged 26 commits into from
May 8, 2020

Conversation

Mattwmaster58
Copy link
Contributor

@Mattwmaster58 Mattwmaster58 commented May 7, 2020

Here's the things I did (which fixes #37 ):

  • refactored code to
  • ran black
  • made function docstring
  • added type hints here and there
  • refactored to allow things like a contrib module

What I didn't do (possibly yet):

  • optimize for performance
    • if doing so, I'd like to use numpy (if it exists) to optimize stuff
      • if that's not possible or/and I dont't feel like that, I guess I could perform micro-optimizations I used to make the code more readable if necessary
  • test deliberate failures (reason for decreased coverage)

What I need to do:

  • update docs

@codecov
Copy link

codecov bot commented May 7, 2020

Codecov Report

Merging #38 into master will decrease coverage by 0.19%.
The diff coverage is 94.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #38      +/-   ##
==========================================
- Coverage   97.67%   97.47%   -0.20%     
==========================================
  Files           1        1              
  Lines         129      119      -10     
==========================================
- Hits          126      116      -10     
  Misses          3        3              
Impacted Files Coverage Δ
pixelmatch/__init__.py 97.47% <94.44%> (ø)

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 7ed84e4...cf3f10d. Read the comment docs.

@Mattwmaster58
Copy link
Contributor Author

Mattwmaster58 commented May 8, 2020

After thinking about this some more, it probably would make more sense to import PIL.Image gracefully and then check with isinstance/subclass. Otherwise, how would the user load the image in the first place?

Copy link
Owner

@whtsky whtsky left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!

It would be nice if you can split the PIL support, general code cleanup and type hints into different PRs -- this can make code review a lot easier for me :)

I just found that there's no type checking in CI right now, I'll add them tonight.

pyproject.toml Outdated Show resolved Hide resolved
pixelmatch.py Outdated Show resolved Hide resolved
pixelmatch.py Outdated Show resolved Hide resolved
pixelmatch.py Outdated Show resolved Hide resolved
pixelmatch.py Outdated Show resolved Hide resolved
@Mattwmaster58 Mattwmaster58 changed the title Major rework: support PIL.Image, more pythonic API, code rework Major rework: more pythonic API, code rework May 8, 2020
@Mattwmaster58
Copy link
Contributor Author

Totally get WRT splitting up PRs. I'll try to get a different one out for PIL support

@Mattwmaster58 Mattwmaster58 requested a review from whtsky May 8, 2020 15:45
@Mattwmaster58
Copy link
Contributor Author

Alright. I removed all the PIL specific code and moved pixelmatch to __init__ to lay the groundwork for a contrib module. And type hinted the rest of the code.

pixelmatch/__init__.py Outdated Show resolved Hide resolved
Copy link
Owner

@whtsky whtsky left a comment

Choose a reason for hiding this comment

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

Also please don't checkin pixelmatch/contrib as they are empty right now.

@Mattwmaster58
Copy link
Contributor Author

Mattwmaster58 commented May 8, 2020

I had a vague idea that git didn't track empty files/folders, turns out it's only empty folders 😄

Copy link
Owner

@whtsky whtsky left a comment

Choose a reason for hiding this comment

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

Looks nice. Can you rebase or merge master and solves the conflict? Thanks

README.md Outdated Show resolved Hide resolved
pixelmatch/__init__.py Outdated Show resolved Hide resolved
@Mattwmaster58
Copy link
Contributor Author

Done

@whtsky
Copy link
Owner

whtsky commented May 8, 2020

@Mattwmaster58
Copy link
Contributor Author

Mattwmaster58 commented May 8, 2020

Fixed Fixed. Wow, black got me multiple times there. Maybe some pre-commit hooks to run black would be in order 😉

test_pixelmatch.py Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@whtsky
Copy link
Owner

whtsky commented May 8, 2020

Maybe some pre-commit hooks to run black would be in order 😉

Personally I use editor's auto format: https://github.com/psf/black#editor-integration

Python's pre-commit requires manual install, and I have some unpleasant experience with it due to Python venv switching.

@whtsky whtsky merged commit ce448fe into whtsky:master May 8, 2020
@whtsky
Copy link
Owner

whtsky commented May 8, 2020

Merged, thanks!

@whtsky
Copy link
Owner

whtsky commented May 8, 2020

@Mattwmaster58
Copy link
Contributor Author

Mattwmaster58 commented May 8, 2020 via email

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.

Make options pythonic
2 participants