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: name temporary variables with namespace #797

Merged
merged 6 commits into from
Mar 2, 2020

Conversation

AirQuick
Copy link
Member

Fixes #777

Before fixing the problem, the temporary variable names were [final name]-doc and [final name]-doc-uri. They can conflict with user-defined variable names.

This pull request changes the naming convention: [private namespace prefix]:[context element local name]-[context element id]-doc and ...-uri.
[private namespace prefix] is local (XQuery) or impl (XSLT).

@AirQuick AirQuick added the bug label Feb 21, 2020
@AirQuick AirQuick added this to the v1.6.0 milestone Feb 21, 2020
@AirQuick AirQuick requested a review from galtm February 21, 2020 05:35
@commit-lint
Copy link

commit-lint bot commented Feb 21, 2020

Tests

  • xquery: reproduce issue 777 (0de58bf)
  • xslt: reproduce issue 777 (b2c2300)
  • replace typo x: with xsl: (f3dcdde)

Bug Fixes

  • name temporary variable names with private namespace (2c5d155)

Contributors

@AirQuick

@AirQuick
Copy link
Member Author

AirQuick commented Feb 22, 2020

@galtm
Before making this change, I refactored the relevant code (in #782 and #792). That's why many lines are different from v1.5.0. I believe the refactoring has made it easier to review this.

test/xspec-777.xspec Outdated Show resolved Hide resolved
@galtm
Copy link
Member

galtm commented Feb 26, 2020

Before making this change, I refactored the relevant code (in #782 and #792). That's why many lines are different from v1.5.0. I believe the refactoring has made it easier to review this.

Thanks, @AirQuick . I've started looking at this. I need a little more time that's not at the end of a long day.

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.

The code changes look good. I reviewed the diffs and also looked at compiler output from your test and some ad hoc variations on it.

The Compilation wiki might need some updates. No hurry. If you're too busy with other pull requests and you'd like me to update the wiki, let me know.

@AirQuick
Copy link
Member Author

AirQuick commented Mar 2, 2020

Thanks for the reivew, @galtm

The Compilation wiki might need some updates. No hurry. If you're too busy with other pull requests and you'd like me to update the wiki, let me know.

I forgot about it! although we have a reminder (#505).
Can you try updating it?

A problem is that there is no .xspec file (apart from the variable files you created) that is connected to the description in Compilation.md. To facilitate updating Compilation.md, it would be nice if we have an actual .xspec file.

@galtm
Copy link
Member

galtm commented Mar 2, 2020

I forgot about it! although we have a reminder (#505).
Can you try updating it?

A problem is that there is no .xspec file (apart from the variable files you created) that is connected to the description in Compilation.md. To facilitate updating Compilation.md, it would be nice if we have an actual .xspec file.

I forgot about #505 . I'll assign it to myself. Do you think it's ready to be worked on anytime? The description says (from over a year ago), "At some point in the future when the outstanding pull requests are resolved".

@AirQuick
Copy link
Member Author

AirQuick commented Mar 2, 2020

Soon I'm going to open a small refactoring pull request to get ready for dynamic invocation. And I was going to ask you to review it. It's small, but it may change something in the compiled stylesheet. I'll let you know.

Another idea is that the compiler itself should possibly write comments like <!-- the main template to run the suite --> into the compiled stylesheet/query in the first place. Just my quick thought.

@galtm
Copy link
Member

galtm commented Mar 2, 2020

OK, I will wait for (at least) your refactoring pull request before working on the compilation wiki. I can review that PR when it's ready.

I tend to lean toward seeing code comments as useful rather than clutter, so I like your idea about making the compiler insert some of the comments that are currently added manually to the compilation wiki.

@AirQuick AirQuick merged commit db31e24 into xspec:master Mar 2, 2020
@AirQuick AirQuick deleted the fix_issue-777 branch March 2, 2020 15:09
@AirQuick
Copy link
Member Author

AirQuick commented Mar 2, 2020

OK, I will wait for (at least) your refactoring pull request before working on the compilation wiki. I can review that PR when it's ready.

Opened #807. Fortunately, I believe nothing has changed in the compiled stylesheet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Variable declaration pollutes other variables
2 participants