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

add os.absolutePath; fixes #8174 #8213

Merged
merged 4 commits into from
Jul 10, 2018
Merged

add os.absolutePath; fixes #8174 #8213

merged 4 commits into from
Jul 10, 2018

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Jul 5, 2018 •
edited
Loading

fixes #8174

NOTE: see #8174 and https://forum.nim-lang.org/t/4004 for prior discussion.
absolutePath would be best put in ospaths but nim doesn't support circular dependencies (yet...).

An alternative would've been to split in 2:

# in ospaths:
proc absolutePath*(path: string, base: string)
# in os:
proc absolutePath*(path: string)

but that's uglier (and less DRY).

Another alternative would've been to add fwd reference of getCurrentDir in ospaths but that causes link errors in case user doesn't explicitly call getCurrentDirbut just calls absolutePath in his code (something nim could improve on IMO )

@timotheecour timotheecour changed the title add os.absolutePath add os.absolutePath; fixes #8174 Jul 5, 2018
@timotheecour timotheecour mentioned this pull request Jul 5, 2018
Closed
This was referenced Jul 5, 2018
Open
Merged
Araq
Araq reviewed Jul 6, 2018
View reviewed changes
lib/pure/os.nim Outdated
@@ -296,6 +296,18 @@ proc setCurrentDir*(newDir: string) {.inline, tags: [].} =
else:
if chdir(newDir) != 0'i32: raiseOSError(osLastError())

proc absolutePath*(path: string, root = getCurrentDir()):string=
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: use a space after the colon

Copy link
Member

@Araq Araq Jul 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Also should have documentation and "runnable examples".

Copy link
Member Author

@timotheecour timotheecour Jul 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

done

@timotheecour
Copy link
Member Author

PTAL

dom96
dom96 reviewed Jul 7, 2018
View reviewed changes
lib/pure/os.nim Outdated
try:
let a = absolutePath("a", "b")
doAssert false, "should've thrown"
except:
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically, assertion failures are not catchable. You should raise an exception instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

actually that try/catch was buggy independently of the fact that assertion failures are not catchable; will fix; thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

@dom96 just wrote #8237 in response to your comment

Copy link
Member Author

Choose a reason for hiding this comment

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

You should raise an exception instead

done

timotheecour
timotheecour commented Jul 7, 2018
View reviewed changes
lib/system.nim Outdated
runnableExamples:
doAssertRaises(ValueError):
raise newException(ValueError, "Hello World")
## specified exception. Example:
Copy link
Member Author

Choose a reason for hiding this comment

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

NOTE: once #8245 is merged, this diff will disappear

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@timotheecour timotheecour mentioned this pull request Jul 7, 2018
Merged
@timotheecour timotheecour mentioned this pull request Jul 9, 2018
Closed
@timotheecour
Copy link
Member Author

@dom96 PTAL

@timotheecour timotheecour mentioned this pull request Jul 10, 2018
Closed
dom96
dom96 requested changes Jul 10, 2018
View reviewed changes
lib/pure/os.nim Outdated
else:
if not root.isAbsolute:
# CHECKME: is that the correct exception type?
raise newException(ValueError, root)
Copy link
Contributor

Choose a reason for hiding this comment

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

This exception type seems fine, but the error message is poor. You need something like "The specified root is not absolute: "

dom96
dom96 approved these changes Jul 10, 2018
View reviewed changes
Copy link
Contributor

@dom96 dom96 left a comment

Choose a reason for hiding this comment

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

Mergeable once tests pass.

@dom96
Copy link
Contributor

dom96 commented Jul 10, 2018

I'm guessing that the test failure is unrelated.

@dom96 dom96 merged commit 25bf0d1 into nim-lang:devel Jul 10, 2018
@timotheecour timotheecour deleted the pr_abspath branch July 10, 2018 20:18
@timotheecour
Copy link
Member Author

I'm guessing that the test failure is unrelated.

=> filed #8267

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Araq Araq Araq left review comments

@dom96 dom96 dom96 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

missing ospaths.absolutePath function to get absolute path from a path
3 participants
@timotheecour @dom96 @Araq