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

Add code for handling conversion from filename or filpath to MIME type #29

Merged
merged 19 commits into from
Feb 2, 2022

Conversation

helenahalldiniths
Copy link
Contributor

This vill fix #12

@helenahalldiniths
Copy link
Contributor Author

Currently there are no checks to confirm that the argument is valid in any of the methods. Maybe this could be added later on?

LukasFinne
LukasFinne previously approved these changes Jan 31, 2022
Copy link
Contributor

@LukasFinne LukasFinne left a comment

Choose a reason for hiding this comment

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

Looks good to me! Everything from using the correct type and sub-type pair to the tests done for the code.

Copy link
Contributor

@kappsegla kappsegla left a comment

Choose a reason for hiding this comment

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

Are the changes to pom.xml needed? The current file already contains specification for java version 17.
The files test.html and test.pfd in test folder, can we do without them? Does Paths.get really need an existing file to work?

@helenahalldiniths helenahalldiniths marked this pull request as draft January 31, 2022 14:29
@helenahalldiniths
Copy link
Contributor Author

The changes in pom.xml were made accedently, good catch! I also removed the files that were used during testing, they are indeed unnecessary.

LukasFinne
LukasFinne previously approved these changes Jan 31, 2022
Copy link
Contributor

@kappsegla kappsegla left a comment

Choose a reason for hiding this comment

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

It's a good practice to end files with a newline. Helps diff tools to see changes at end of file.
https://stackoverflow.com/questions/4700312/empty-new-line-at-the-end-of-the-java-source-files

Co-authored-by: Martin Blomberg <martin.blomberg@outlook.com>
helenahalldiniths and others added 2 commits January 31, 2022 19:59
Co-authored-by: Martin Blomberg <martin.blomberg@outlook.com>
…st.java

Co-authored-by: Martin Blomberg <martin.blomberg@outlook.com>
@helenahalldiniths
Copy link
Contributor Author

Good to know!

@kappsegla
Copy link
Contributor

So what happens if we have a filetype not in the list? Is there a default type like text or something we could use?
https://developer.mozilla.org/en-US/docs/Web/HTTP/Basics_of_HTTP/MIME_types/Common_types
Two primary MIME types are important for the role of default types:

text/plain is the default value for textual files. A textual file should be human-readable and must not contain binary data.
application/octet-stream is the default value for all other cases. An unknown file type should use this type. Browsers pay a particular care when manipulating these files, to protect users from software vulnerabilities and possible dangerous behavior.

@helenahalldiniths
Copy link
Contributor Author

Support for default MIME type have been added

@Vimbayinashe Vimbayinashe self-requested a review February 2, 2022 10:25
@Vimbayinashe
Copy link
Contributor

This looks great @helenahalldiniths!

I am not sure if we should add conversions for the common file types such as .doc, .docx, .xls & .xls as well since we do not have a default type text/plain. At the same time I am wary of creating a long list in FormatConverter with all the possible extensions.

String[] result = file.split("\\.");
String subtype = result[result.length - 1];
Optional<String> type = Optional.ofNullable(subtypeToType.get(subtype));
if (type.isEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this could be simplified using one of the methods on Optional type variables like orElse orElseGet ? Might need to store the complete type string and not only subtype maybe for that to work?

@Vimbayinashe
Copy link
Contributor

This looks great @helenahalldiniths!

I am not sure if we should add conversions for the common file types such as .doc, .docx, .xls & .xls as well since we do not have a default type text/plain. At the same time I am wary of creating a long list in FormatConverter with all the possible extensions.

This can always be updated according to the file types that we need.

@helenahalldiniths helenahalldiniths merged commit c24d57c into main Feb 2, 2022
@helenahalldiniths helenahalldiniths deleted the issue12 branch February 2, 2022 14:10
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.

MIME types
4 participants