Skip to content

Conversation

@fviernau
Copy link
Member

This can be helpful for development.

@fviernau fviernau requested a review from a team as a code owner May 31, 2023 07:04
@codecov
Copy link

codecov bot commented May 31, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.41 ⚠️

Comparison is base (c0015b4) 64.61% compared to head (8d55dda) 64.21%.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #7068      +/-   ##
============================================
- Coverage     64.61%   64.21%   -0.41%     
  Complexity     1969     1969              
============================================
  Files           331      333       +2     
  Lines         16654    16759     +105     
  Branches       2387     2394       +7     
============================================
  Hits          10761    10761              
- Misses         4846     4951     +105     
  Partials       1047     1047              
Flag Coverage Δ
funTest-non-docker 29.35% <ø> (ø)
test 40.07% <0.00%> (-0.44%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
.../src/main/kotlin/commands/ConvertOrtFileCommand.kt 0.00% <0.00%> (ø)
...lin/commands/DownloadResultsFromPostgresCommand.kt 0.00% <0.00%> (ø)
.../commands/ExtractRepositoryConfigurationCommand.kt 0.00% <0.00%> (ø)
.../main/kotlin/commands/GetPackageLicensesCommand.kt 0.00% <0.00%> (ø)
...in/kotlin/commands/ListLicenseCategoriesCommand.kt 0.00% <0.00%> (ø)
...li/src/main/kotlin/commands/ListPackagesCommand.kt 0.00% <0.00%> (ø)
...lin/commands/SetDependencyRepresentationCommand.kt 0.00% <0.00%> (ø)
...src/main/kotlin/commands/TransformResultCommand.kt 0.00% <0.00%> (ø)
...s/classifications/LicenseClassificationsCommand.kt 0.00% <0.00%> (ø)
...per-cli/src/main/kotlin/commands/dev/DevCommand.kt 0.00% <0.00%> (ø)
... and 7 more

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@fviernau fviernau force-pushed the rewrite-assets-helper branch 2 times, most recently from f32ce7a to ca3177f Compare May 31, 2023 08:20
@fviernau fviernau force-pushed the rewrite-assets-helper branch from ca3177f to 260b795 Compare May 31, 2023 11:29
@fviernau fviernau requested a review from sschuberth May 31, 2023 11:31
@fviernau fviernau force-pushed the rewrite-assets-helper branch from 260b795 to dfb7278 Compare May 31, 2023 12:39
@fviernau fviernau requested a review from sschuberth May 31, 2023 12:39
@fviernau fviernau enabled auto-merge (rebase) May 31, 2023 12:40
@fviernau fviernau force-pushed the rewrite-assets-helper branch from dfb7278 to eaca3de Compare May 31, 2023 13:28
@fviernau fviernau requested a review from sschuberth May 31, 2023 13:32
@fviernau fviernau force-pushed the rewrite-assets-helper branch from c3f3f19 to 8d55dda Compare June 2, 2023 13:22
Comment on lines +104 to +105
// Paths to nodes in the tree of JsonNodes, whose subtree shall not be changed. Explicitly ignoring these subtrees is
// needed in order to avoid bringing in default values property values which are not present in the original file.
Copy link
Member

Choose a reason for hiding this comment

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

I've being thinking about the rationale once more. What confused me so far is the emphasis of (arbitrary) "default values". But actually, what I believe you mean is the machine-specific values of the config and environment classes that are used by default. When rewriting files, you do not want to update e.g. the amount of available memory with the number from the machine running the rewrite command, but you simply want to keep what's in the file, correct?

If so, I propose the following wording:

Some serialized classes contain information specific to the machine the serialization was run on. When reserializing files, nodes containing such information should not be updated but taken as-is.

Copy link
Member

Choose a reason for hiding this comment

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

what I believe you mean is the machine-specific values of the config and environment classes

Yet more detailed, if machine-specific values would be used, the replacing back of dummy values to placeholders would not work, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

When rewriting files, you do not want to update e.g. the amount of available memory with the number from the machine running the rewrite command, but you simply want to keep what's in the file, correct?

Yes, I simply want to keep the subtree as-is. I'm not sure what you exactly refer to with "machine-specific", but in my view referring to "default values" in general perfectly fits here. This for example also refers to

val enableRepositoryPackageConfigurations: Boolean = false,
/**
* Enable the usage of project-local package curations from the [RepositoryConfiguration]. If set to true, apply
* package curations from a local .ort.yml file before applying those specified via the command line i.e. curations
* from the .ort.yml take precedence.
*/
val enableRepositoryPackageCurations: Boolean = false,
/**
* Force overwriting of any existing output files.
*/
val forceOverwrite: Boolean = false,
/**
* The license file patterns.
*/
val licenseFilePatterns: LicenseFilePatterns = LicenseFilePatterns.DEFAULT,
: Do you consider "machine-specific" still to be more appropriate?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yet more detailed, if machine-specific values would be used, the replacing back of dummy values to placeholders would not work, right?

I don't understand this question. Can you elaborate a bit?

Copy link
Member

Choose a reason for hiding this comment

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

My thinking was that you first replace e.g.

`"\"<REPLACE_MAX_MEMORY>\""` with `"33445504"`

and later again replace

`"33445504"` with `"\"<REPLACE_MAX_MEMORY>\""`

and in between those two replacements "33445504" would get replaced with the real amount of memory of the machine running the command, the back-replacing would not work anymore. But giving it a second thought, I believe this cannot happen, exactly because these values are being patched, i.e. they are present in the file, and thus no default value is used for them.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what you exactly refer to with "machine-specific"

I'm referring to default values, esp. in Environment, that vary from machine to machine, like the maxMemory property. I had a feeling that the real reason for the code is that you only want to not take those properties. But if you really also do not want to take properties like enableRepositoryPackageConfigurations, then the term "machine-specific" obviously is not applicable.

* License-Filename: LICENSE
*/

package org.ossreviewtoolkit.helper.commands.dev
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Maybe spell out the package (and directory) name as "development" as other similar grouping commands don't use abbreviations either. For the command itself, I acknowledge "orth dev ..." is nice to be short.

@fviernau fviernau requested a review from sschuberth June 5, 2023 10:42
@fviernau fviernau merged commit f39a261 into main Jun 5, 2023
@fviernau fviernau deleted the rewrite-assets-helper branch June 5, 2023 11:01
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