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

Accept YML #9

Merged
merged 3 commits into from
Feb 24, 2021
Merged

Accept YML #9

merged 3 commits into from
Feb 24, 2021

Conversation

ek08
Copy link
Contributor

@ek08 ek08 commented Feb 23, 2021

Signed-off-by: Ehtesham ehteshamkhan3014@gmail.com
Verify should support file extension "yml" #8

Signed-off-by: Ehtesham <ehteshamkhan3014@gmail.com>
* @author Gary O'Neall
*
*/
public class SpdxToolsHelper {

public enum SerFileType {
JSON, RDFXML, XML, XLS, XLSX, YAML, TAG
JSON, RDFXML, XML, XLS, XLSX, YAML, TAG, YML
Copy link
Member

Choose a reason for hiding this comment

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

Rather than add an additional file type, we should just map the .yml file extension to the existing YAML file type. The SerFileType should represent type type of file (e.g. the format of the content), not the extension used. This would the change more straight forward.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have done the requested changes but here I have a doubt that if I don't add the YML file type then I will get this error
an enum switch case label must be the unqualified name of an enumeration constant

@@ -64,6 +64,7 @@
temp.put("yaml", SerFileType.YAML);
temp.put("tag", SerFileType.TAG);
temp.put("spdx", SerFileType.TAG);
temp.put("yml", SerFileType.YML);
Copy link
Member

Choose a reason for hiding this comment

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

This could just be mapped to SerFileType.YAML

Signed-off-by: Ehtesham <ehteshamkhan3014@gmail.com>
@ek08
Copy link
Contributor Author

ek08 commented Feb 23, 2021

I think I should revert back to the initial commit @goneall

@@ -81,7 +82,8 @@ public static ISerializableModelStore fileTypeToStore(SerFileType fileType) thro
case XLSX: return new SpreadsheetStore(new InMemSpdxStore(), SpreadsheetFormatType.XLSX);
case XML: return new MultiFormatStore(new InMemSpdxStore(), Format.XML, Verbose.COMPACT);
case YAML: return new MultiFormatStore(new InMemSpdxStore(), Format.YAML, Verbose.COMPACT);
default: throw new InvalidSPDXAnalysisException("Unsupporte file type: "+fileType+". Check back later.");
case YML: return new MultiFormatStore(new InMemSpdxStore(), Format.YAML, Verbose.COMPACT);
Copy link
Member

Choose a reason for hiding this comment

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

You no longer need this case statement. The additional file type will get taken care of by the line SerFileType retval = EXT_TO_FILETYPE.get(ext); below..

Signed-off-by: Ehtesham <ehteshamkhan3014@gmail.com>
@goneall
Copy link
Member

goneall commented Feb 24, 2021

Thanks @ek08

@goneall goneall merged commit 8846a18 into spdx:master Feb 24, 2021
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