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 schema error & in turn per-suite UI data provision #3217

Merged
merged 1 commit into from
Jul 15, 2019

Conversation

sadielbartholomew
Copy link
Collaborator

@sadielbartholomew sadielbartholomew commented Jul 13, 2019

These changes partially address #3216 (the table that is not rendered returns, populated with the suite's data, but the GraphQL error on 'state' is still there & needs further investigation).

This PR essentially reverts #3211 (bar one line), because whilst not X to test for a sequence being non-empty in a conditional is the Pythonic way advocated by PEP8, in this context the logic on field_ids needs to distinguish between an empty list & None (which that form does not do, as both are "falsy").

This is because field_ids = getattr(root, field_name, None) returns None as the default, & seems to be doing so for all the suites I have tried out to test this (as observed by adding in a print() function to check), whereas the query field argument default is an empty list (e.g. as here).

However, for the remaining line from the one commit of that PR, it is safe to use a not X construct for workflows as X because if workflows is None: is explicitly tested in the previous statement, which itself cannot return a None value:

https://github.com/kinow/cylc-flow/blob/67199ccc5c7abaa4f162f221e796adf5c5c83bc2/cylc/flow/network/schema.py#L699-L706

hence that line is safe & good.

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).
  • Off-template: Integration testing is important 😬!**
  • No change log entry required (e.g. change is small or internal only).
  • No documentation update required for this change.

@sadielbartholomew sadielbartholomew added the bug Something is wrong :( label Jul 13, 2019
@sadielbartholomew sadielbartholomew added this to the cylc-8.0a1 milestone Jul 13, 2019
@sadielbartholomew sadielbartholomew self-assigned this Jul 13, 2019
@kinow
Copy link
Member

kinow commented Jul 13, 2019

Hi @sadielbartholomew !

We are here again! Déjà vu in cylc-flow haha, I remember breaking cylc-flow for the exact same reason, and you spotted it. Thanks, and thanks for the thorough explanation.

@kinow
Copy link
Member

kinow commented Jul 14, 2019

Off-template: Integration testing is important!**

+1 or even a simple unit testing just for some of these methods.

@sadielbartholomew
Copy link
Collaborator Author

It turns out that this PR fixes the missing table aspect of the corresponding Issue, but the GraphQL errors relating to 'state', raised as returned errors for queries & as a traceback in the terminal, remain there, so are not related to this. I've just edited the opening comment to this PR accordingly.

I will look into the cause of those errors tomorrow, if someone else doesn't diagnose the problem before then (feel free to!).

@oliver-sanders oliver-sanders merged commit 60031dd into cylc:master Jul 15, 2019
@sadielbartholomew sadielbartholomew deleted the i3216-fix-schema branch July 15, 2019 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is wrong :(
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants