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

Refactor and add ConversionConfig class as argument to createDcat #24

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jonasseglare
Copy link

@jonasseglare jonasseglare commented May 12, 2023

Refactor and add ConversionConfig class as argument to createDcat

Description

This pull request addresses issue #22 and makes it possible to specify a working directory where files are produced when calling the method Manager.createDcat. As part of that, I made the following changes:

  • Add a new class ConversionConfig that contains (1) the apiSpecMap and (2) the workDir working directory. This class wraps the data consumed by Manager.createDcat.
  • Move some of the logic from the methods Manager.createFromFile and Manager.createFromDirectory to this new ConversionConfig class as methods fromDirectory and fromFile.
  • Make the Manager.createDcat implementation take a ConversionConfig instead of a MultiValueMap.
  • Output files to the workDir of the ConversionConfig.
  • Fix a bug in the Manager.createFromList method where the exception message will be pushed twice to results if createDcat throws an exception. This bug was fixed as a consequence of the refactoring.

Fixes #22

Discussion

The motivation for this PR was that I wanted to add a way of specifying the path where files are produced, as a workDir parameter. For that, I added the ConversionConfig class that now groups the apiSpecMap and workDir parameter. However, the workDir parameter could also be part of the Manager. I am unsure about what is best. If we would move workDir from the ConversionConfig class to Manager, then the ConversionConfig class would only contain one instance variable, apiSpecMap, and wouldn't be necessary anymore. At the same time, I am not sure it is a bad idea to have a ConversionConfig class that wraps the apiSpecMap parameter, because it makes clarifies intent and makes the code more easily extendable to adding more parameters.

Checklist

  • My contributions and commit messages follows the style guidelines of this project
  • I have made corresponding changes to the documentation
  • New and existing unit tests pass locally with my changes
  • The Pull Request has an informative and human-readable title
  • Changes are limited to a single goal (avoid scope creep)
  • Code can be automatically merged (no conflicts)
  • I confirm that I have read any Contribution guidelines (CONTRIBUTING)
  • I confirm that I wrote and/or have the right to submit the contents of my PR, by agreeing to the Developer Certificate of Origin, by adding a 'sign-off' to my commits

e.printStackTrace();
}
results.add(new Result(null, result));
Copy link
Author

Choose a reason for hiding this comment

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

In the description of the PR, I mentioned a bug that I fixed: In case we enter the catch branch above, then the same exception message is going to be pushed a second time to results because of this line. That is probably not what we want.

The updated code should address that issue.

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.

Parametrisera katalogen där filer ska produceras
1 participant