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.

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

signac view fails when statepoints contain forwardslashes #213

Closed
klywang opened this issue Jul 25, 2019 · 6 comments
Closed

signac view fails when statepoints contain forwardslashes #213

klywang opened this issue Jul 25, 2019 · 6 comments
Assignees
Labels
bug Something isn't working good first issue Good for newcomers
Milestone

Comments

@klywang
Copy link
Contributor

klywang commented Jul 25, 2019

Description

signac view cannot create folders when statepoints contain forward slashes (ie, if a statepoint value is '/oasis/scratch/some_file.txt')

To reproduce

signac view

Example of statepoint:
{'pot_file': '/oasis/scratch/path/to/file/some_file.out'}

Error output

Error: [Errno 13] Permission denied: '/oasis'### System configuration

Please complete the following information:

  • Operating System [e.g. macOS]: macOS Moajve (10.14.4)
  • Version of Python [e.g. 3.7]: 3.7.3
  • Version of signac [e.g. 1.0]: 1.2.0
@vyasr
Copy link
Contributor

vyasr commented Jul 25, 2019

I think the proper behavior here is to just throw a more descriptive error message. Right now it just gives a permission denied error in this case, we should at least change it to raise an Exception that informs the user why it doesn't work. I don't think that this is necessarily something that we should support, because it would require modifying the values to avoid containing file separators.

@vyasr vyasr added the bug Something isn't working label Jul 25, 2019
@vyasr vyasr added this to the v1.2.1 milestone Jul 25, 2019
@csadorf
Copy link
Contributor

csadorf commented Jul 26, 2019

I agree that adding a more descriptive error message and potentially suggesting a work-around is the way to go. We have been pretty liberal about what a state point key and value may contain as long as it is JSON-encodable and issues like that are kind of bound to happen.

My suggested solution is to explicitly check for os.sep in keys and values and simply error out. A potential work-around is to only include keys and values that do not contain these values.

@klywang Would that be a satisfactory solution?

@klywang
Copy link
Contributor Author

klywang commented Jul 26, 2019

@csadorf Yeah that sounds good!

@mikemhenry
Copy link
Collaborator

Just to clarify the behavior we want:
paths are okay in state points but will prevent view from working (with a helpful error message)

@vyasr
Copy link
Contributor

vyasr commented Sep 4, 2019

Yes, I think that's the best option. We could potentially disallow them in 2.0, but I don't think it's necessary since it only causes problems for views.

@vyasr
Copy link
Contributor

vyasr commented Nov 6, 2019

Resolved by #233.

@vyasr vyasr closed this as completed Nov 6, 2019
@bdice bdice modified the milestones: v1.2.1, v1.3.0 Dec 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

6 participants