-
Notifications
You must be signed in to change notification settings - Fork 67
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
Fixes for 0.18.1 #62
Fixes for 0.18.1 #62
Conversation
tests/tester.nim
Outdated
else: | ||
result.add(c[0]) | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When writing code like this, instead of wrapping the code in an if, just write the following above the code:
if c.len() == 0:
return
It's the same and it makes the code less messy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
try: | ||
eraseLine() | ||
except OSError: | ||
discard |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't a valid fix for whatever issue this fixes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This happens when running tests - there's no terminal available so eraseLine()
raises OSError.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then how have the tests worked before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only Windows raises OSError, Linux does not so I can only guess that this wasn't tested on Windows before.
quotedArgs = quoted_args.map((x: string) => ("\"" & x & "\"")) | ||
for i in 0..quotedArgs.len-1: | ||
if " " in quotedArgs[i]: | ||
quotedArgs[i] = "\"" & quotedArgs[i] & "\"" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a reason to do this. Quoting everything is fine, and the code is much more succinct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but it is failing tests even on 0.18.0 since it is appending spaces or " to file paths for --nimbleDir or --choosenimDir which then no longer resolve as paths. It's easier fixing the tester than changing choosenim parseCliParams()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, how did this pass before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It didn't pass for me even on 0.18.0, I think it is probably because of running the tests on Windows. It never passed for me because the command line params were getting messed up.
Verified tests pass on latest #head of Nim on both Windows and Linux. I require the patched
Invalid integer is raised by |
Fixes for ""[0]