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

Merge lists using a for-loop instead of unionBy #779

Merged
merged 1 commit into from
Sep 23, 2021

Conversation

kinow
Copy link
Member

@kinow kinow commented Sep 20, 2021

These changes close #778

Requirements check-list

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Appropriate tests are included (unit and/or functional).
  • No change log entry required (why? e.g. invisible to users, not released yet).
  • No documentation update required.

@kinow kinow added this to the 0.6.0 milestone Sep 20, 2021
@kinow kinow self-assigned this Sep 20, 2021
@codecov-commenter
Copy link

codecov-commenter commented Sep 20, 2021

Codecov Report

Merging #779 (6899398) into master (d86f687) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #779      +/-   ##
==========================================
+ Coverage   90.97%   90.98%   +0.01%     
==========================================
  Files          84       84              
  Lines        1673     1675       +2     
  Branches      105      105              
==========================================
+ Hits         1522     1524       +2     
  Misses        121      121              
  Partials       30       30              
Flag Coverage Δ
e2e 77.85% <66.66%> (-0.04%) ⬇️
unittests 81.66% <100.00%> (+0.02%) ⬆️

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

Impacted Files Coverage Δ
src/graphql/merge.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d86f687...6899398. Read the comment docs.

@kinow kinow marked this pull request as ready for review September 21, 2021 01:15
}`
const merged = mergeQueries(queryA, queryB)
const expected = gql`query A {
jobs (workflows: ["root", "test", "airplane"]) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is essentially what was happening when merging the subscriptions; see linked issue for the complete diff.

@kinow
Copy link
Member Author

kinow commented Sep 21, 2021

To review:

  1. Go to master, build it for development
  2. Open a tree view
  3. Add a second tree view, watch either Network tab for the stop/start messages, or see if the UI clears the views and re-populates them

Then switch to this branch and try that again. The subscription must not change since you are adding a second tree view.

Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

Code looks good, haven't tested yet (my clones are in a mess).

Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

Tested, the subscription wasn't stopped / restarted 👍.

@kinow kinow changed the title Merge lists using a Set/unique Merge lists using a for-loop instead of unionBy Sep 21, 2021
@hjoliver hjoliver merged commit de5ca75 into cylc:master Sep 23, 2021
@kinow kinow deleted the merge-unique-list-values branch September 23, 2021 06:21
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.

Subscription being reloaded unnecessarily
4 participants