Calculate located resources #2005

antonbauhofer · 2023-09-18T16:23:46Z

Summary of changes

Calculate located resources and resources with located children

Introduce conditions to check the entries in the locate popup
Adjust the state to only store paths for resources with located children
Consider short- and full names of frequent licenses
Fix and add tests
code style improvements

Context and reason for change

We are introducing a feature to "locate" resources that have or contain attributions with criteria that can be set in a new popup.

How can the changes be tested

Run the tests

Note: Please review the guidelines for contributing to this repository.

MarkusObendrauf · 2023-09-18T17:17:26Z

src/Frontend/state/helpers/action-and-reducer-helpers.ts

+  getExternalAttributionsToResources,
+  getFrequentLicensesNameOrder,
+} from '../selectors/all-views-resource-selectors';
+import { State } from '../../types/types';


I really dont think we should be importing this stuff here.

Yes, I agree with you. Actually, I had kind of a similar comment on the original diff. I will change the structure to avoid this.

MarkusObendrauf · 2023-09-18T17:28:32Z

src/Frontend/state/helpers/action-and-reducer-helpers.ts

@@ -254,3 +264,96 @@ export function getIndexOfAttributionInManualPackagePanel(

  return packageCardIndex !== -1 ? packageCardIndex : null;
 }
+
+export function locateResourcesByCriticalityAndLicense(


Not sure about this structure. Here's how I understand these helpers:

In resource-reducer.ts we have a bunch of 'setters' and 'updaters'. These are all mostly a couple of lines - they take an input and write it to the state. If some processing is needed to get the data into the right form, that is done in a helper function here, so that the resource-reducer.ts function remains concise.

With that in mind, I think the structure should be the following:

In resource-reducers.ts, replace the function setResourcesWithLocatedAttributions with a new function updateResourcesWithLocatedAttributions.

The new function takes as input criticality and licenseNames

Inside action-and-reducer-helpers, we have a function with signature (criticality, licensenames, state) -> (resourcesWithLocatedChildren, locatedResources). It does pretty much what this function does but doesnt update state. (not 100% sure about passing state here, but it seems reasonable an is done elsewhere.)

Then updateResourcesWithLocatedAttributions just calls the helper function and updates the state with the return value.

more generally, imo, when your code does read -> calculate -> write then its nice (and very unit-testable) to have the calculations independent of the reading & writing.

Yes, I am with you here. I had the same thought, but just didn't fix it yet. I will change the structure as you suggested.

MarkusObendrauf · 2023-09-18T17:39:36Z

src/Frontend/state/helpers/action-and-reducer-helpers.ts

+      const attribution = attributions[attributionId];
+      if (
+        (criticalityMatches(criticality, attribution) &&
+          !licenseIsDifferent(licenseNames, attribution)) ||


i dont like double negatives, and i dont understand what is happening here. i guess theres a difference between licenseMatches and !licenseisDifferent and that dfference is key to this thing's correctness. i find this extemely confusing.

i think id write the condition as something like
(licenseMatches || !LicenseIsSet) && (criticalityMatches || !criticalityIsSet)
(possibly with some helpfully named intermediate variable)

I was playing with the same ideas, but actually, the logic you are suggesting is not correct, since it would pass if neither the license, nor the criticality is set.

The point is that there are three possible states for each variable: Matching, not matching and not set. This will also be true when we introduce additional criteria. We want to evaluate as true, if any one of the criteria is matching and none of the other is not matching. With that in mind, I still think the way it is currently written is good.
But I can add a comment to clarify things.

MarkusObendrauf · 2023-09-18T17:51:18Z

src/Frontend/state/helpers/action-and-reducer-helpers.ts

+  attribution: PackageInfo,
+): boolean {
+  return (
+    criticality != SelectedCriticality.Any &&


might be clearer if written as
criticality != initialResourceState.locatePopup.selectedCriticality

Not sure if I agree. We could also have SelectedCriticality.Any as a constant with a meaningful name like unspecifiedCriticality or similar.

MarkusObendrauf

.

MarkusObendrauf · 2023-09-20T07:43:22Z

src/Frontend/Components/LocatorPopup/__tests__/LocatorPopup.test.tsx

+    clickOnButton(screen, 'Apply');
+
+    const expectedLocatedResources = {
+      resourcesWithLocatedChildren: new Set(['pathToMITHigh/']),


in the real world, all paths begin with a /, and we expect the path '/' to be in resourceWithLocatedChildren. could you add this to the test?

MarkusObendrauf · 2023-09-20T07:45:12Z

src/Frontend/Components/LocatorPopup/__tests__/LocatorPopup.test.tsx

+  const testResourcesToAttributions: ResourcesToAttributions = {
+    'pathToMITHigh/': ['MITHighAttribution'],
+    'pathToMITHigh/pathToMITMedium': ['MITMediumAttribution'],
+    pathToApacheHigh: ['ApacheHighAttribution'],


can we add quotation marks around all of these?

Fixes opossum-tool#1944. - Introduce conditions to check the entries in the locate popup - Adjust the state to only store paths for resources with located children - Consider short- and full names of frequent licenses - Fix and add tests - code style improvements Signed-off-by: Anton Bauhofer <anton.bauhofer@tngtech.com>

MarkusObendrauf

lgtm

MarkusObendrauf self-requested a review September 18, 2023 17:17

MarkusObendrauf self-assigned this Sep 18, 2023

MarkusObendrauf reviewed Sep 18, 2023

View reviewed changes

MarkusObendrauf requested changes Sep 18, 2023

View reviewed changes

antonbauhofer force-pushed the calculate-located-resources branch from 2efe37f to e436395 Compare September 18, 2023 18:59

lennartclaas mentioned this pull request Sep 19, 2023

Add logic to license locator icons in ProjectStatisticsPopup #2006

Merged

antonbauhofer force-pushed the calculate-located-resources branch from 8f99c7b to 2b30ea0 Compare September 19, 2023 12:07

antonbauhofer mentioned this pull request Sep 19, 2023

Add logic to calculate located resources #1978

Closed

antonbauhofer force-pushed the calculate-located-resources branch from 2b30ea0 to 1756a85 Compare September 19, 2023 12:13

MarkusObendrauf requested changes Sep 20, 2023

View reviewed changes

antonbauhofer force-pushed the calculate-located-resources branch from 1756a85 to 3cea374 Compare September 20, 2023 07:57

antonbauhofer force-pushed the calculate-located-resources branch from 994e92d to 30b9baf Compare September 20, 2023 08:27

MarkusObendrauf approved these changes Sep 20, 2023

View reviewed changes

antonbauhofer merged commit fb3f2c8 into opossum-tool:main Sep 20, 2023

antonbauhofer deleted the calculate-located-resources branch September 27, 2023 09:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Calculate located resources #2005

Calculate located resources #2005

antonbauhofer commented Sep 18, 2023

MarkusObendrauf Sep 18, 2023

antonbauhofer Sep 18, 2023

MarkusObendrauf Sep 18, 2023

MarkusObendrauf Sep 18, 2023

antonbauhofer Sep 18, 2023

MarkusObendrauf Sep 18, 2023 •

edited

Loading

antonbauhofer Sep 18, 2023

MarkusObendrauf Sep 18, 2023

antonbauhofer Sep 18, 2023

MarkusObendrauf left a comment

MarkusObendrauf Sep 20, 2023

MarkusObendrauf Sep 20, 2023

MarkusObendrauf left a comment

Calculate located resources #2005

Calculate located resources #2005

Conversation

antonbauhofer commented Sep 18, 2023

Summary of changes

Context and reason for change

How can the changes be tested

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkusObendrauf Sep 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkusObendrauf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkusObendrauf left a comment

Choose a reason for hiding this comment

MarkusObendrauf Sep 18, 2023 •

edited

Loading