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.

Sign up for GitHub

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

Jump to bottom

Do not parse dates as prices. Sort imports. #19

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

Do not parse dates as prices. Sort imports. #19

wants to merge 11 commits into from

Conversation

bulatbulat48
Copy link

#4

@codecov
Copy link

codecov bot commented Aug 18, 2019 •
edited
Loading

Codecov Report

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

@@          Coverage Diff          @@
##           master    #19   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           3      3           
  Lines          95    114   +19     
  Branches       21     26    +5     
=====================================
+ Hits           95    114   +19
Impacted Files Coverage Δ
price_parser/parser.py 100% <100%> (ø) ⬆️

Gallaecio
Gallaecio reviewed Aug 19, 2019
View reviewed changes
price_parser/parser.py Show resolved Hide resolved
tests/test_price_parsing.py Show resolved Hide resolved
Gallaecio
Gallaecio reviewed Sep 16, 2019
View reviewed changes
Copy link
Member

@Gallaecio Gallaecio left a comment

Choose a reason for hiding this comment

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

Changes seem great in general, now we just need to try and keep test coverage to 100%

price_parser/parser.py Outdated Show resolved Hide resolved
price_parser/parser.py Outdated Show resolved Hide resolved
@bulatbulat48 bulatbulat48 mentioned this pull request Oct 14, 2019
Open
kmike
kmike reviewed Oct 18, 2019
View reviewed changes
tests/test_price_parsing.py Outdated
'$', '10', 10),

Example(None, '$10 EUR at March, 2016 or 2019-08-19',
'$', '10', 10),
Copy link
Member

@kmike kmike Oct 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

hey @bulatbulat48! All these tests are added to PRICE_PARSING_EXAMPLES_XFAIL, i.e. a list of examples which are known not to work. It means that if they fail, test suite is green. Could you please move them to PRICE_PARSING_EXAMPLES_BUGS_CAUGHT?

Copy link
Author

@bulatbulat48 bulatbulat48 Oct 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Got it. Moved tests. Thank you for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kmike kmike kmike left review comments

@Gallaecio Gallaecio Gallaecio approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bulatbulat48 @kmike @Gallaecio