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: Fully support storing content-type in file extensions #268

Merged
merged 2 commits into from Oct 14, 2020

Conversation

joachimvh
Copy link
Member

Resolves #146 .

Makes sure to update the file path if the content-type gets updated and makes the metadata paths be independent of the content-type.

Also renamed all references from "data resource" to "document" for consistency.

@joachimvh joachimvh added the 🐛 bug Something isn't working label Oct 14, 2020
const link = await this.resourceMapper.mapUrlToFilePath(identifier, metadata.contentType);

// Check if we already have a corresponding file with a different extension
await this.verifyExistingExtension(link);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondering if there are cases where something could go wrong inbetween this and the writing of the file.
The chance this could happen is probably very small, but in case it could, should we somehow move this verification (and removal) to after the writing of the file?
It's better to have two files after a crash than none at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

True, there's something to be said for that. The mapper is going to start crashing (or behaving weirdly, no idea) once there are two such files though but at least you still have your data I guess?

Copy link
Member Author

Choose a reason for hiding this comment

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

On second thought going to leave it as is since this would require more rewriting than I first thought. Can always be changed later if really required.

@joachimvh joachimvh merged commit e861b08 into master Oct 14, 2020
@joachimvh joachimvh deleted the file-extensions branch October 14, 2020 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add file extension to match content-type
2 participants