Robbepop / apint

Arbitrary precision integers library.
Other
26 stars 4 forks source link

Name changes and indentation fix #22

Closed AaronKutch closed 6 years ago

AaronKutch commented 6 years ago

I found some places in yours and my code that had space indentation and converted those. Most checked were converted to wrapping which is numerically correct. I checked the code base over about 3 times and you should also check for any mistakes I made and run all the unit tests (there are some unit tests being ignored and I am not sure how to turn those on). If you have any other name changes you want, now is the time. I remember looking through the codebase a few weeks ago and finding some docs where you said you wanted a better name for the function.

AaronKutch commented 6 years ago

I think we should fix all of the issues that involve breakage now before releasing a 0.3

AaronKutch commented 6 years ago

Also, could you add some docs specifying that the shift ops are wrapping but return an error when the shift is more than or equal to the bit width?

coveralls commented 6 years ago

Coverage Status

Coverage remained the same at 70.409% when pulling a073ed6593e1abc78e35e1ea0ff079eea9e94f6b on AaronKutch:iss17 into abde5c2f20d8f8381605b993bb2d8f57c4f7cd36 on Robbepop:master.

codecov-io commented 6 years ago

Codecov Report

Merging #22 into master will not change coverage. The diff coverage is 52.92%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master     #22   +/-   ##
======================================
  Coverage    70.3%   70.3%           
======================================
  Files          23      23           
  Lines        4358    4358           
======================================
  Hits         3064    3064           
  Misses       1294    1294
Impacted Files Coverage Δ
src/apint/serialization.rs 93.86% <ø> (ø) :arrow_up:
src/apint/relational.rs 29% <0%> (ø) :arrow_up:
src/int.rs 0% <0%> (ø) :arrow_up:
src/uint.rs 0% <0%> (ø) :arrow_up:
src/apint/shift.rs 100% <100%> (ø) :arrow_up:
src/apint/bitwise.rs 72.53% <12.76%> (ø) :arrow_up:
src/apint/arithmetic.rs 81.41% <70.49%> (ø) :arrow_up:
src/digit.rs 89.26% <96.87%> (ø) :arrow_up:
src/apint/to_primitive.rs 97.89% <97.82%> (ø) :arrow_up:

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 abde5c2...a073ed6. Read the comment docs.

Robbepop commented 6 years ago

Thank you again for all the work you currently put into the apint crate!!

I found some places in yours and my code that had space indentation and converted those.

Since the Rust style-guides strictly tell to use 4-spaces instead of tabs maybe we should stick to the guidelines and do the same. Link: https://github.com/rust-lang-nursery/fmt-rfcs/blob/master/guide/guide.md#indentation-and-line-width

Also, could you add some docs specifying that the shift ops are wrapping but return an error when the shift is more than or equal to the bit width?

Let's open an issue for that and I will do that whenever I find time. :)

Also: Would it be possible (feasible) for you to separate this gigantic Pull-Request into two pull-requests: One for the renaming and one for the code formatting?

AaronKutch commented 6 years ago

You mentioned using the style guideline in the last pull request and I assumed that because your codebase used tabs that the style guideline was to use tabs. Just merge this pull request because it has all tabs, then in a second pull request I will only convert the tabs to spaces

AaronKutch commented 6 years ago

After that pull request is merged, then start fixing the issues

Robbepop commented 6 years ago

Okay I will merge it and we will proceed as you said. Sorry about the confusion about the code formatting - I guess I wasn't so sure about the details myself back then. Thanks again for all the hard work! Keep it up!! :)

AaronKutch commented 6 years ago

Oh and I plan on also converting ... to ..= in a third pull request. Are there any other reformatting changes we want to make?

Robbepop commented 6 years ago

Oh and I plan on also converting ... to ..= in a third pull request. Are there any other reformatting changes we want to make?

Where exactly is ... used where it should be ..= ?

For the code formatting changes: I think it is just the best to closely stick to the recommendations.

AaronKutch commented 6 years ago

Actually there are only 8 places where it should be changed, I'll just add on a small commit for it

AaronKutch commented 6 years ago

Do you get notifications when I accidentally try to push directly to your repository? If so, it is probably just me setting my remotes wrong or some other error.

Robbepop commented 6 years ago

I think I do not get any notifications and nothing happens since you have no privileges to push to my master. ;)