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

Fix #9871 by making jl_substrtod/f whitespace-tolerant #9783

Merged
merged 1 commit into from
Jan 18, 2015
Merged

Fix #9871 by making jl_substrtod/f whitespace-tolerant #9783

merged 1 commit into from
Jan 18, 2015

Conversation

IainNZ
Copy link
Member

@IainNZ IainNZ commented Jan 15, 2015

No description provided.

@IainNZ IainNZ mentioned this pull request Jan 15, 2015
Closed
@IainNZ IainNZ force-pushed the ird/fix9781 branch 2 times, most recently from 130ecbb to d35b030 Compare January 15, 2015 14:52
@jakebolewski
Copy link
Member

Do we want this behavior? As a datapoint, Python allows for trailing whitespace.

>>> int("1\n")
1
>>> int("1 ")
1

@IainNZ
Copy link
Member Author

IainNZ commented Jan 15, 2015

I like it, makes parsing files a lot easier (e.g. parsing a line floats from a CSV manually doesn't require a chomp). The current situation is broken/inconsistent, at least. Can I get an OK-to-merge from someone? This maybe should be backported if we are still doing that.

@jakebolewski
Copy link
Member

Doesn't jl_substrtof have the same inconsistency?

@IainNZ
Copy link
Member Author

IainNZ commented Jan 15, 2015

Good point! Pushed amended commit

@IainNZ IainNZ changed the title Fix #9871 by making jl_substrtod whitespace-tolerant Fix #9871 by making jl_substrtod/f whitespace-tolerant Jan 15, 2015
jakebolewski
jakebolewski reviewed Jan 16, 2015
View reviewed changes
test/strings.jl
# float(SubString) wasn't tolerant of trailing whitespace, which was different
# to "normal" strings. This also checks we aren't being too tolerant and allowing
# any arbitrary trailing characters.
@test float("1\n") == 1.0
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be changed to float64, but otherwise lgtm.

Copy link
Member Author

Choose a reason for hiding this comment

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

It does call float64 though on all systems, right?

Copy link
Member

Choose a reason for hiding this comment

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

It does, just thought it would be good to be explicit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I'll add it

@tkelman
Copy link
Contributor

tkelman commented Jan 18, 2015

There was an unusual Travis failure here for OSX that looked like #9572, but someone restarted the build and now the log with the failure is gone. If a failure log is worth keeping, it might be preferable to restart the build via rebasing rather than restarting through Travis' interface.

@IainNZ
Copy link
Member Author

IainNZ commented Jan 18, 2015

I restarted it, sorry. Assumed it was a known issue. It did look like that though, you're right.

IainNZ added a commit that referenced this pull request Jan 18, 2015
@IainNZ
Merge pull request #9783 from JuliaLang/ird/fix9781
ad489ed 
Fix #9871 by making jl_substrtod/f whitespace-tolerant
@IainNZ IainNZ merged commit ad489ed into master Jan 18, 2015
@jiahao jiahao deleted the ird/fix9781 branch January 18, 2015 07:23
@pao
Copy link
Member

pao commented May 8, 2015

Is this a backport candidate? #9781 (comment)

@tkelman
Copy link
Contributor

tkelman commented May 8, 2015

I think it should be. Needs careful conflict resolution in backporting the tests, since there are other tests nearby for recent utf8proc and reverseind which would likely fail on release-0.3.

IainNZ added a commit that referenced this pull request May 9, 2015
@IainNZ @tkelman
Fix #9871 by making jl_substrtod whitespace-tolerant
66f081d 
(cherry picked from commit 64e2dbd)
ref PR #9783

Conflicts:
	test/strings.jl
@tkelman
Copy link
Contributor

tkelman commented May 9, 2015

Backported in 66f081d - someone speak up before we tag 0.3.9 in a few weeks if there's any backwards compatibility concerns about this (or any other backport for that matter). Apparently nobody was paying attention to the

This maybe should be backported if we are still doing that.

bit a few months ago, whoops.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@IainNZ @jakebolewski @tkelman @pao