-
-
Notifications
You must be signed in to change notification settings - Fork 524
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
Always expand all paths (journals, templates, etc) #1468
Conversation
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 looks great. Just one comment about the new functions to fix.
Also, would you mind adding a test or two? I think just a couple of unit tests for the new functions should be more than enough.
return os.path.expanduser("~") | ||
|
||
|
||
def expand_path(path): |
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 think we could do with just home_dir
and expand_path
as long as expand_path
also runs abspath
. Would you mind merging expand_path
and absolute_path
into one function?
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 tried that at first because I assumed that it wouldn't make a difference but I think that's not a great idea for a couple reasons.
- It breaks the tests as currently written. A few BDD tests check the console output of the application and the output that gets parsed contains relative paths. That means that those tests would have to changed to parse the absolute path which is a bit more complicated.
- The absolute path is only needed once so it would add extra work to all the other calls.
- Console error messages are more readable when they display the relative path instead of the absolute path.
e8bde7b
to
516b005
Compare
I'm currently running into some problems creating tests that work for both platforms. Essentially, the call to |
a3699dc
to
7a86bbc
Compare
This also fixed bugs with relative journal and template paths.
7a86bbc
to
b6448cd
Compare
Also, make the tests specific to Windows, Mac & Linux Co-authored-by: Micah Jerome Ellison <micah.jerome.ellison@gmail.com>
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 went ahead and updated the tests to fix the issues you were running into.
The rest looks good. Thank you!
This also fixed bugs with relative journal and template paths.
Checklist
for the same issue.
This is related to the discussion in issue #998. I took a look at the path expansion code used throughout the project and decided to create a new
path.py
file to hold the three common cases.~
).~
). Note: This used to happen in the opposite order which is probably what caused problems.Additionally, a call to
expand_path
is now made before loading template files which solves the problem with loading files like~/template
. I'm still not sure though if the error message after failing to load a template should print the original template path string or the expanded one (line 210 injrnl/jrnl.py
).Closes #998.