Handles : in path names (+ more for 0.15.1)
#174
Conversation
`:` isn't supported by windows machines. Therefore install doesn't work on windows. We need to therefore: 1. update demo data 2. change tracking client to not allow `:` in the path to be present.
We also don't copy over if they had the old ones.
burr/tracking/client.py
Outdated
| @@ -85,7 +85,10 @@ def __init__( | |||
|
|
|||
| @classmethod | |||
| def get_storage_path(cls, project, storage_dir) -> str: | |||
| return os.path.join(os.path.expanduser(storage_dir), project) | |||
| # need to replace `:` with `_` for the project_id; it becomes part of the path and characters | |||
There was a problem hiding this comment.
I don't think this is the right solution. Better to error if it doesn't match. We can fix upstream, or add a patch to have demo_ instead of demo:
burr/tracking/client.py
Outdated
| # need to replace `:` with `_` for the project_id; it becomes part of the path and characters | ||
| # need to work on windows, etc. | ||
| sanitized_project_id = project.replace(":", "_") | ||
| return os.path.join(os.path.expanduser(storage_dir), sanitized_project_id) |
There was a problem hiding this comment.
issue is that unbeknown to people the project name passed into the tracker gets turned into a path. so either we enforce what a project name can be, or we still need this and instead save the project name as part of metadata and not rely on the UI to assume that the directory name maps to the project name -- that would be longer term fix.
There was a problem hiding this comment.
This is a side-effect of the storage for now and not something we should design around. Can add an error if its windows.
Really, we should be escaping it and this whole problem would go away, but I don't have access to a windows box to test.
There was a problem hiding this comment.
I think it's a bad design assumption on our part to assume the project name is something that becomes part of the file path verbatim. If the project name has / in it, etc. then there's more to sanitize, etc.
There was a problem hiding this comment.
Yeah, its sloppy. As we discussed in person we'll add guards on the project name (fail earlier).
Not too concerned (the local tracking client is not prod-ready anyway), but we want to reduce onboarding friction.
elijahbenizzy
force-pushed
the
fix_windows_install
branch
from
14f6840
to
15da73d
Compare
We use the project name as the filename so we have to guard it. This is a bit hacky, but we'll decouple them later.
Nothing respects dark mode except for one button which was hard to find in dark mode. This turns it off for now. TODO -- turn it back on!
elijahbenizzy
changed the title
: in path names: in path names (+ more for 0.15.1)
:isn't supported by windows machines.Therefore install doesn't work on windows.
We need to therefore:
:in the path to be present.[Short description explaining the high-level reason for the pull request]
Changes
How I tested this
Notes
Checklist