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

Fix for Configuration & Status not populated Fully #2738

Merged
merged 3 commits into from
Aug 22, 2024

Conversation

Sn0w3y
Copy link
Contributor

@Sn0w3y Sn0w3y commented Aug 8, 2024

This pull request offers two alternative solutions to fix the issue in the listActiveComponents method.

Option 1: Use push Instead of concat

Changes

components.push(...this.getComponentsByFactory(factory.id));

Explanation

In this approach, we use the push method to add elements to the components array directly.

Advantages

  • Simplicity: The code is straightforward and easy to understand.
  • Performance: push modifies the array in place, which is generally faster than creating a new array with concat.

Disadvantages

  • In-Place Modification: push modifies the original array, which might not be desirable if you need to maintain immutability.

Option 2: Assign the Result of concat

Changes

public listActiveComponents(ignoreComponentIds: string[] = []): CategorizedComponents[] {
    const allComponents = [];
    const factories = this.listAvailableFactories();
    for (const entry of factories) {
        let components: EdgeConfig.Component[] = [];
        for (const factory of entry.factories) {
            components = components.concat(...this.getComponentsByFactory(factory.id));
        }
        allComponents.push({
            category: entry.category,
            components: components,
        });
    }
    // (rest of the code remains the same)
}

Explanation

In this approach, we use concat to combine the current components array with new elements and assign the result back to components.

Advantages

  • Immutability: concat does not modify the original array; instead, it returns a new array. This can be useful if you want to keep the original array unchanged.

Disadvantages

  • Performance: concat creates a new array each time, which may be less efficient than push for large arrays.
  • Complexity: This approach adds a bit more complexity by reassigning the array.

Copy link

codecov bot commented Aug 8, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Additional details and impacted files
@@              Coverage Diff              @@
##             develop    #2738      +/-   ##
=============================================
- Coverage      56.03%   55.89%   -0.13%     
- Complexity      8044     8047       +3     
=============================================
  Files           2060     2077      +17     
  Lines          87538    87939     +401     
  Branches        6426     6493      +67     
=============================================
+ Hits           49041    49146     +105     
- Misses         36813    37108     +295     
- Partials        1684     1685       +1     

@lukasrgr
Copy link
Contributor

lukasrgr commented Aug 13, 2024

@Sn0w3y i fell over this issue as well, Array.prototoype.concat should be used for merging arrays, with concat we have to write some unneccessary code which doesnt make anything better, so i would use push all day long

@Sn0w3y
Copy link
Contributor Author

Sn0w3y commented Aug 13, 2024

@lukasrgr can be merged, removed Option 2 concat :)

@Sn0w3y
Copy link
Contributor Author

Sn0w3y commented Aug 19, 2024

@sfeilmeier can be merged i guess

@lukasrgr lukasrgr merged commit d1b1012 into OpenEMS:develop Aug 22, 2024
3 of 4 checks passed
@Sn0w3y Sn0w3y deleted the fix-components-status&config branch October 1, 2024 12:28
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