-
Notifications
You must be signed in to change notification settings - Fork 18
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 resource setup widget bugs #661
Merged
edan-bainglass
merged 5 commits into
aiidalab:master
from
edan-bainglass:fix-resource-setup-widget
Jan 9, 2025
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
8f8b658
Add additional guards for missing code setup input
edan-bainglass f5bd632
Disable quick setup button if missing any requisites
edan-bainglass fcb06aa
Apply uniqueness to missing template variables warning
edan-bainglass 1dca354
Make `ResourceSetupBaseWidget` "public"
edan-bainglass 1a11fab
Fix code full label uniqueness check
edan-bainglass File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this change needed? orm.Code is deprecated so would be nice to avoid if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, Code worked where AbstractCode did not. But I think maybe InstalledCode should also work. Question is, do we support the other Code flavors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear, AbstractCode does not work, allowing codes of existing full labels to go through!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, why doesn't it work though? Is this a limitation of QueryBuilder? Or a bug in aiida-core?
In your testing, are the preexisting codes of
Code
type orInstalledCode
type?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect they are all InstalledCode instances, but not sure. Will doubt check in the morning. It could be a QB issue. In any case, if Code works and covers all use cases and no resolution is found by tomorrow, I'll proceed with it for now, as this PR is holding up the release of the QE app. We can always revisit when we know more. Unless you think the use of Code is present breaking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, seems like a QB limitation. When using
AbstractCode
, thenode_type
field of the QB dictionary is{'like': 'data.code.abstract.%'}
, whereas forCode
, it is{'like': 'data.core.code.%'}
.All my instances are of
InstalledCode
, which have anode_type
ofdata.core.code.installed.InstalledCode
. I don't believe'data.code.abstract'
is actually anode_type
of any of theCode
flavors. All code entry points aredata.core.code...
.For now (due to time constraints), I will proceed with
Code
. Will open an issue+PR in aiida-core to address this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aiidateam/aiida-core#6687 for reference