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

Add black #17

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add black #17

wants to merge 4 commits into from

Conversation

manycoding
Copy link
Contributor

@manycoding manycoding commented Jul 18, 2019

This project has special style, but I am for consistency first.

And to save the time on any future styling comments.

@codecov
Copy link

codecov bot commented Jul 18, 2019

Codecov Report

Merging #17 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #17   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           3      3           
  Lines          93     93           
  Branches       21     21           
=====================================
  Hits           93     93
Impacted Files Coverage Δ
price_parser/__init__.py 100% <ø> (ø) ⬆️
price_parser/parser.py 100% <100%> (ø) ⬆️
price_parser/_currencies.py 100% <100%> (ø) ⬆️

@kmike
Copy link
Member

kmike commented Jul 22, 2019

hey! I like some of the changes, but don't like others. For example, tests became harder to read - previously in Example convention was "input data on a first line, expected results on a second", and now it is a mess, as all these fields look very similar.

@manycoding
Copy link
Contributor Author

manycoding commented Jul 22, 2019

@kmike On my taste these tests are harder to read anyway - you cannot really collapse them. I prefer traditional params like here https://github.com/scrapinghub/price-parser/pull/18/files#diff-091825c38112ff1dafc081caf2916d27R1991.

At the first, we can put tests in ignore.

Why does this Price string hint work? https://github.com/scrapinghub/price-parser/blob/master/price_parser/parser.py#L28

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.

2 participants