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

feat: Generate unique and descriptive filenames for download #3021

Merged
merged 10 commits into from
Oct 29, 2024

Conversation

fhennig
Copy link
Contributor

@fhennig fhennig commented Oct 17, 2024

resolves #2327
resolves #2689

preview URL: https://feat-2327-better-download.loculus.org

Summary

Generates filenames like cchf_nuc_2024-10-23T1225.fasta for the files that are downloaded from the DownloadDialog.

PR Checklist

  • All necessary documentation has been adapted.
  • The implemented feature is covered by an appropriate test.

@fhennig fhennig self-assigned this Oct 17, 2024
@fhennig fhennig added the preview Triggers a deployment to argocd label Oct 17, 2024
@fhennig fhennig marked this pull request as ready for review October 18, 2024 07:23
@corneliusroemer
Copy link
Contributor

The preview url doesn't work because we only put the first 25 characters in the URL, if you use shorter branch names it's easier to guess the branch name. But you can look the actual url up in argocd:
Brave Browser 2024-10-18 15 37 17

@corneliusroemer corneliusroemer force-pushed the feat/2327-better-download-filenames branch from d7d8b67 to e627588 Compare October 18, 2024 13:38
Copy link
Contributor

@corneliusroemer corneliusroemer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff, works well, just realized that my spec wasn't entirely complete re casing irregularity - I think changing camelCase to kebab-case would be good, and probably using "sequences" instead of "unaligned-nucleotide-sequences"

Otherwise the file names get possibly quite too long

image

@fhennig
Copy link
Contributor Author

fhennig commented Oct 18, 2024

Thanks for the feedback, I already thought that we might iterate on the exact filename, but I was happy with getting all the components in place. I agree on the casing stuff and the filename being to long, I like all your suggestions!

@fhennig fhennig force-pushed the feat/2327-better-download-filenames branch from e627588 to a22b063 Compare October 21, 2024 11:26
Copy link
Contributor

@corneliusroemer corneliusroemer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized we don't indicate segments/genes in sequence file names. Might be nice to add that after the respective seq type:

nuc-L or aa-aligned-2K

This only applies to segmented nucs and AAs (if it's main then leave it out)

@fhennig fhennig force-pushed the feat/2327-better-download-filenames branch from dd9cd7c to 89f3a63 Compare October 29, 2024 08:51
@fhennig fhennig force-pushed the feat/2327-better-download-filenames branch from 6017fa9 to c53a0ff Compare October 29, 2024 13:17
Copy link
Contributor

@corneliusroemer corneliusroemer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, just some small non-blocking suggested! We can iterate on the rest as we go, nothing blocking!

@fhennig fhennig merged commit a07649a into main Oct 29, 2024
19 checks passed
@fhennig fhennig deleted the feat/2327-better-download-filenames branch October 29, 2024 13:36
@fhennig
Copy link
Contributor Author

fhennig commented Oct 29, 2024

Thanks for the review!

I didn't make these test changes as I prefer to keep unit tests with rather less than more levels of indirection and templating.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview Triggers a deployment to argocd
Projects
None yet
3 participants