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.

Sign up for GitHub

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

Jump to bottom

fix: pending variable declarations #1452

Merged
merged 8 commits into from
May 10, 2021
Merged

fix: pending variable declarations #1452

merged 8 commits into from
May 10, 2021

Conversation

AirQuick
Copy link
Member

In response to #1415 (comment), this pull request changes how x:variable and x:param[parent::x:scenario] in a pending scenario are handled:

  • They produce no variables, if they are considered as pending.
  • They are not considered as pending, if they are inherited by a focused scenario.

@AirQuick AirQuick added the bug label Apr 14, 2021
@AirQuick AirQuick added this to the v2.2 milestone Apr 14, 2021
@AirQuick AirQuick mentioned this pull request Apr 14, 2021
Merged
@AirQuick AirQuick requested a review from galtm May 2, 2021 07:45
galtm
galtm approved these changes May 4, 2021
View reviewed changes
Copy link
Member

@galtm galtm left a comment

Choose a reason for hiding this comment

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

Is it worthwhile to have tests similar to the ones you have, except that the variables and parameters are defined inside shared scenarios and referenced using x:like? It would show that the inheritance logic accounts for resolution of x:like instead of being based only on the original XSpec document's tree structure. If you'd like me to create those variations (in a separate PR because I don't think I know how to add files to someone else's PR), let me know.

src/compiler/base/main.xsl Show resolved Hide resolved
test/external_unfocused-param_not-inherited-by-focus.xspec Show resolved Hide resolved
@AirQuick AirQuick requested a review from galtm May 9, 2021 17:40
@AirQuick
Copy link
Member Author

AirQuick commented May 9, 2021

Is it worthwhile to have tests similar to the ones you have, except that the variables and parameters are defined inside shared scenarios and referenced using x:like? It would show that the inheritance logic accounts for resolution of x:like instead of being based only on the original XSpec document's tree structure. If you'd like me to create those variations (in a separate PR because I don't think I know how to add files to someone else's PR), let me know.

Good point. I didn't think about that. Can you create them?
I think, if you prefer, you can push commits directly to this PR by cloning its branch. i.e. git clone -b discard-pending-vardecls https://github.com/AirQuick/xspec.git. But a separate PR would be easier to handle.

@galtm
Copy link
Member

galtm commented May 10, 2021

a separate PR would be easier to handle

Sure, I'll do that.

@AirQuick AirQuick enabled auto-merge (squash) May 10, 2021 12:03
@AirQuick AirQuick merged commit 0f65c8b into xspec:master May 10, 2021
@AirQuick AirQuick deleted the discard-pending-vardecls branch May 10, 2021 14:43
galtm pushed a commit to galtm/xspec that referenced this pull request May 10, 2021
Test pending variable declarations referenced by x:like
2ca3e67 
Imitate tests from PR xspec#1452, except that the variables and parameters are
defined inside shared scenarios and referenced using x:like. Shows that
the inheritance logic accounts for resolution of x:like instead of being
based only on the original XSpec document's tree structure.
@galtm galtm mentioned this pull request May 10, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@galtm galtm galtm approved these changes

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
v2.2
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@AirQuick @galtm