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

PLANNER-1752: Move frontend to Immutable.js #530

Merged

Conversation

Christopher-Chianelli
Copy link
Collaborator

JIRA

https://issues.redhat.com/browse/PLANNER-1752

Referenced pull requests

How to retest this PR or trigger a specific build:
  • for a pull request build please add comment: Jenkins retest this
  • for a full downstream build please add comment: Jenkins run fdb
  • for a compile downstream build please add comment: Jenkins run cdb
  • for a full production downstream build please add comment: Jenkins execute product fdb
  • for an upstream build please add comment: Jenkins run upstream

@yurloc
Copy link
Member

yurloc commented Nov 20, 2020

Jenkins retest this.

Copy link
Member

@yurloc yurloc left a comment

Choose a reason for hiding this comment

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

First round of review. Minor issues detected.

My IDE shows errors on some parts of the code but typecheck passes. I need to investigate this.

Otherwise looks nice. Minimal code changes given the API transition.

});
it('remove employee', () => {
expect(
reducer(state.employeeList, actions.removeEmployee(deletedEmployee)),
).toEqual({ ...state.employeeList,
employeeMapById: mapWithoutElement(storeState.employeeList.employeeMapById, deletedEmployee) });
employeeMapById: storeState.employeeList.employeeMapById.delete(deletedEmployee.id as number) });
Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, you can say delete(deletedEmployee.id!). The exclamation mark is a non-null assertion. It's more concise and maybe a more precise type assertion. But we have an ESLint rule that complains about that. Dunno which of the two is worse.

@Christopher-Chianelli
Copy link
Collaborator Author

@yurloc thoughts on the current state of the PR?

Copy link
Member

@yurloc yurloc left a comment

Choose a reason for hiding this comment

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

Sorry for taking so long, I missed the update last week.

LGTM, one small comment.

Using the ?? operator allows us to throw
errors instead of erasing type infomation
and sliently ignoring any problem until it
is too late.
All usage in the UI already converts
the list into a mutable array, and
they do not modify the mutable array,
so it much more useful for the selector
to directly return the mutable array.
@sonarcloud
Copy link

sonarcloud bot commented Dec 11, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

89.7% 89.7% Coverage
0.0% 0.0% Duplication

@Christopher-Chianelli Christopher-Chianelli merged commit d946580 into kiegroup:master Dec 11, 2020
@Christopher-Chianelli Christopher-Chianelli deleted the PLANNER-1752 branch March 24, 2021 15:20
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