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

Merged
merged 2 commits into from
Nov 11, 2024

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.

@joncameron
Copy link
Contributor

joncameron commented Nov 11, 2024

I think it's OK to merge this in without the extra step of decoupling, and I'll create a new issue for adding a new field. Storing the original filename in a separate field sounds like a good change.

Issue: #6109

@cjcolvar cjcolvar merged commit 7933ded into develop Nov 11, 2024
2 checks passed
@cjcolvar cjcolvar deleted the cjcolvar-patch-3 branch November 11, 2024 21:16
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.

3 participants