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

Don't remove file_location since it is used to render the file's filename #6098

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

cjcolvar
Copy link
Member

@cjcolvar cjcolvar commented Nov 4, 2024

Thanks to John Merritt of University of Louisville for pointing out this issue in avalon slack.

Copy link
Contributor

@masaball masaball left a comment

Choose a reason for hiding this comment

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

This change looks good. It does make me think though should we consider uncoupling the direct association of section name and file location? It feels a little weird to continue storing a file location when the file has actually been removed.

@cjcolvar
Copy link
Member Author

cjcolvar commented Nov 4, 2024

I think we're still storing a file location when we move the file and the file might then get moved to tape off disk. So I'm thinking retaining it here is somewhat analogous. If a section title is manually entered then we use that for section name but fallback to file location. I'm wondering if we should store original filename in a separate field to get that decoupling that you're suggesting.

@masaball
Copy link
Contributor

masaball commented Nov 4, 2024

That's a good point about the files being moved to tape storage being analogous...

If we were to decouple, I think storing original filename as a separate field would probably be the easiest/most sensible approach.

I am fine with either approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants