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

Finalize the utils module refactoring #4671

Merged
merged 13 commits into from
Nov 8, 2021
Merged

Conversation

sschuberth
Copy link
Member

Please have a look at the individual commit messages for the details.

@sschuberth sschuberth force-pushed the finalize-utils-refactoring branch from 723f807 to a82648b Compare November 5, 2021 21:52
@sschuberth sschuberth marked this pull request as ready for review November 6, 2021 07:11
@sschuberth sschuberth requested review from oheger-bosch and a team as code owners November 6, 2021 07:11
*/
fun archive(inputDir: File, zipFile: File, prefix: String = ""): Result<File> {
return runCatching {
fun archive(inputDir: File, zipFile: File, prefix: String = ""): File =
Copy link
Member

Choose a reason for hiding this comment

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

From the commit message, it is not clear to me why the strategy for error handling needs to be changed here. Kotlin's Result class should be independent on other utility functions. Maybe this becomes obvious in later commits, but it would be helpful to have a hint in the commit message.

Copy link
Member Author

@sschuberth sschuberth Nov 8, 2021

Choose a reason for hiding this comment

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

My reason for this change was that I needed to remove the dependency on showStackTrace(), which left the onFailure branch empty. So the function then only did an internal wrapping with runCatching, which I felt should be something that the caller should do, if desired. Also, it allowed to use a slightly more elegant syntax to return the zipFile.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for clarifying.

This is a fixup for 9c9d138.

Signed-off-by: Sebastian Schuberth <[email protected]>
Refactor some functions to not depend on classes from `core-utils`
anymore as a preparation for moving `ArchiveUtils` to `common-utils`.

Signed-off-by: Sebastian Schuberth <[email protected]>
This class can be used independently of ORT. Note that `core-utils` still
needs to depend on `commons-compress` for its
`XZCompressedLocalFileStorage`.

Signed-off-by: Sebastian Schuberth <[email protected]>
This removes the warning log when a version requirement is not satisfied
and the user configured to ignore this, but that should be fine in the
context of upcoming changes that will relax the version requirements on
external tools.

Signed-off-by: Sebastian Schuberth <[email protected]>
@sschuberth sschuberth force-pushed the finalize-utils-refactoring branch 2 times, most recently from 82c40cd to 01c3830 Compare November 8, 2021 09:43
These are all functional in the sense that no mocking but real I/O is
applied.

Signed-off-by: Sebastian Schuberth <[email protected]>
Slightly modify the class so it can be used independently of ORT.

Signed-off-by: Sebastian Schuberth <[email protected]>
Slightly modify the class so it can be used independently of ORT.

Signed-off-by: Sebastian Schuberth <[email protected]>
Slightly modify the code so it can be used independently of ORT.

Signed-off-by: Sebastian Schuberth <[email protected]>
Slightly modify the class so it can be used independently of ORT.

Signed-off-by: Sebastian Schuberth <[email protected]>
…ils`

This function can be used independently of ORT.

Signed-off-by: Sebastian Schuberth <[email protected]>
@sschuberth sschuberth merged commit b878b30 into master Nov 8, 2021
@sschuberth sschuberth deleted the finalize-utils-refactoring branch November 8, 2021 17:11
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