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

Check lower base limit in base_repr. #6298

Merged
merged 3 commits into from
Feb 1, 2016

Conversation

nbeaver
Copy link
Contributor

@nbeaver nbeaver commented Sep 10, 2015

A minor fix to ensure the range is as expected. (This is my first pull request, so let me know if I missed something in the protocol.)

@jaimefrio
Copy link
Member

It would be nice to add a test that the error is properly raised somewhere here. Also, since you are working on this function, the docstring comment on it only handling positive numbers seems to be wrong, would be nice if you could fix that too. Otherwise, LGTM.

@charris
Copy link
Member

charris commented Sep 21, 2015

Should squash the commits. Use `git rebase -i HEAD^^' in the branch and follow directions, then force push to origin.

@charris charris changed the title Check lower base limit Check lower base limit in base_repr. Sep 21, 2015
@charris
Copy link
Member

charris commented Sep 30, 2015

Also needs a test for raising the error. Should go in numpy/core/tests/test_numeric.py.

@rgommers
Copy link
Member

@nbeaver could you please address the comments? It would be good to get this PR merged.

@nbeaver
Copy link
Contributor Author

nbeaver commented Feb 1, 2016

@jaimefrio @charris I have squashed the commits, fixed the docstring, and added the test as requested. There was some kind of merge problem, but I rebased the recent master and was able to fix it.

@rgommers Thanks for the reminder.

charris added a commit that referenced this pull request Feb 1, 2016
Check lower base limit in base_repr.
@charris charris merged commit 5a02591 into numpy:master Feb 1, 2016
@charris
Copy link
Member

charris commented Feb 1, 2016

Thanks @nbeaver .

@rgommers rgommers added this to the 1.12.0 release milestone Feb 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants