Skip to content

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