Conversation
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||
tE3m
left a comment
tE3m
left a comment
There was a problem hiding this comment.
Good changes! Not sure about all allowed suffixes though - take a look :)
| name="file_path", | ||
| label="MaxQuant intensities file (proteinGroups.txt)", | ||
| value=None, | ||
| accept=".txt,.tsv,.csv", |
There was a problem hiding this comment.
This is a tricky one. While any of csv, tsv, txt are possible, only txt makes sense, right? MQ will always output the file as proteingroups.txt, so does it make sense to allow the other ones?
There was a problem hiding this comment.
Same goes for all the other tool-specific imports (DIA-NN, MSFragger, MQ peptides/evidence)
There was a problem hiding this comment.
While you are correct, I wouldn't be overly strict here. Rather allow more file types that could be parsed and not unnecessarily restrict the user.
I could imagine there's at least one biologist out there who has MaxQuant-like protein data in a file that doesn't end in .txt.
There was a problem hiding this comment.
I get that and I wouldn't suggest restricting it further if this implementation actually didn't allow selecting files that don't adhere to the accept specification. At least on all machines I tested, the file picker allows ignoring the accept spec. I'm also fine with leaving as is.
There was a problem hiding this comment.
Yeah sorry, I didn't phrase it properly. I know that it also allows other file types, but at least on my OS, it requires an extra click. This is at most a minor inconvenience, so it wouldn't be a big deal to implement your proposal.
There was a problem hiding this comment.
I made it a bit stricter. Some definitely take and expects tsv, here I only took out the csv because txt seems to be some kind of standard everywhere. For peptide, MaxQuant and Evidence I restricted it to only txt. What do you think?
3 participants
Description
fixes #266
Before the user could choose any file for the file import although we had a file picker implemented for the frontend but did not use it in the backend. The user should now only be able to import the file types we want them to. (Well not really, there is a way around it - but at least it is visible which file types are expected.)
Changes
One little line in forms.py and everywhere where we use the File Import Field.
Testing
Create a run and check out any step that requires you to upload a file.
PR checklist
Development
Mergeability
blackpnpm formatand checked withpnpm lintCode review