-
Notifications
You must be signed in to change notification settings - Fork 24
fix: adds simple check on empty filename #786
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
Conversation
jsam
left a comment
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 would add a test which caused this problem here as well, but I have no idea which DOI caused it, so any help with searching of that DOI is more than welcome. 🙉
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 would add a test which caused this problem here as well, but I have no idea which DOI caused it, so any help with searching of that DOI is more than welcome. 🙉
git grep path .renku/datasets shows that the empty path is in .renku/datasets/dd38afca-0e96-4034-a2e5-dc7505c0af30/metadata.yml so I guess the DOI is https://doi.org/10.5281/zenodo.3348115 (https://zenodo.org/record/3348115).
| if p and len(p) >= 1: | ||
| return Path(p).name | ||
|
|
||
|
|
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.
Can you please explain how this fix works? If filename is None then no DatasetFile object will be created!?
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 it should be created, but filename field will remain None within the instantiated object.
The problem of why raise was happening is because this Path(None) results in TypeError. Now why that is None is probably during import phase something went wrong or we didn't properly validate.
Hmm, this is interesting. Seems like migration dumped that dataset since I don't even have it. 🤔 Thanks! I'll look into this and add the test. |
30a391f to
5b1b7f8
Compare
rokroskar
left a comment
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'm just looking at what happens to the DatasetFile objects that have name: null and they seem to also have a very strange url:
- '@type':
- prov:Entity
- schema:DigitalDocument
- wfprov:Artifact
_id: blob/c6a0aa165dcab4ca7f5c45a935f1d8e1a1eab52b/
_label: c6a0aa165dcab4ca7f5c45a935f1d8e1a1eab52b
_project: null
added: '2019-07-30T09:21:18.055457+01:00'
based_on: null
creator: []
dataset: Binary black-hole surrogate waveform catalog
name: null
path: ''
url:
- https
- zenodo.org
- /api/files/f6c6c4f3-f63e-43fd-a16b-5629af025bac/remnant_fits/fit_3dq8.h5
- ''
- ''
- ''
Perhaps this gets corrupted somewhere?
|
I still see the same issue with the |
rokroskar
left a comment
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.
Thanks, seems to work perfectly now!
closes: #781