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

new parse_fraction function for simple fractions added with test data #60

Merged
merged 11 commits into from
Mar 24, 2021

Conversation

Manish-210
Copy link
Contributor

@Manish-210 Manish-210 commented Feb 25, 2021

See issue #59
parse_fraction function handles basic fraction.
Some test data has also been added.

Resolves #59

@codecov
Copy link

codecov bot commented Feb 25, 2021

Codecov Report

Merging #60 (4cebc90) into master (18aa853) will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #60      +/-   ##
==========================================
+ Coverage   98.70%   98.78%   +0.07%     
==========================================
  Files          85       86       +1     
  Lines         309      328      +19     
  Branches       55       60       +5     
==========================================
+ Hits          305      324      +19     
  Misses          1        1              
  Partials        3        3              
Impacted Files Coverage Δ
number_parser/__init__.py 100.00% <100.00%> (ø)
number_parser/parser.py 98.35% <100.00%> (+0.13%) ⬆️

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.

What about a mention in the README?

@Manish-210
Copy link
Contributor Author

Thanks, I have included a few examples in the README

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.

At the moment we have an introductory paragraph for the Usage section that mentions 3 interfaces, followed by 4 numbered interface sections.

I think it may be best to remove that paragraph, and rename those sections to something like:

  • Converting numbers in-place
  • Parsing a number
  • Parsing an ordinal
  • Parsing a fraction

adding bullets in readme and changing introductory paragraph
@Manish-210
Copy link
Contributor Author

Renaming the sections is good, also I have changed the introductory paragraph a little in the usage section. Let me know if it's ok or not.

README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
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.

Some minor wording issues, and I think this is ready to merge.

README.rst Outdated Show resolved Hide resolved
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.

Looks great to me!

@Manish-210
Copy link
Contributor Author

Will there be any other checks before merging this pull request?

@Gallaecio
Copy link
Member

Let’s wait for @noviluni to find some time to review this as well. If he finds the changes OK as well, we’ll merge.

Copy link
Contributor

@noviluni noviluni left a comment

Choose a reason for hiding this comment

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

@Manish-210 Good job! 🚀

Sorry for my late review. I really like the approach you used and I think it will be really useful. Thanks for updating the README too.

I added some notes, let me know once you address them and I will proceed with merging this 😄

number_parser/parser.py Show resolved Hide resolved
number_parser/parser.py Outdated Show resolved Hide resolved
number_parser/parser.py Outdated Show resolved Hide resolved
number_parser/parser.py Outdated Show resolved Hide resolved
@Manish-210
Copy link
Contributor Author

Thanks, @noviluni, let me know any other changes.

number_parser/parser.py Outdated Show resolved Hide resolved
@noviluni noviluni merged commit c834854 into scrapinghub:master Mar 24, 2021
@noviluni
Copy link
Contributor

Thanks @Manish-210! 😄

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.

Support for simple fractions
3 participants