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

[urlrewrite] Minor edits #906

Merged
merged 3 commits into from
Dec 20, 2023
Merged

Conversation

joewiz
Copy link
Member

@joewiz joewiz commented Mar 20, 2023

  • the /tools root is no longer in eXist
  • the sandbox tool is no longer in eXist; adapted to discuss eXide
  • added a few essential details to the picture of URL processing and controller-config
  • incorporate recent changes to roots in Improve controller-config inline comments exist#4807
  • reorder $exist:* variables from most general to most specific components
  • add descriptions of common uses of URL rewriting

@joewiz joewiz requested a review from a team March 20, 2023 02:59
@joewiz joewiz force-pushed the url-rewrite-edits branch from 8519916 to 8107ce1 Compare March 20, 2023 03:05
@joewiz
Copy link
Member Author

joewiz commented Mar 20, 2023

(I rebased this PR after merging #903, but CI tests are still failing on this PR. The tests in README.md - mvn validate, mvn test, and npm run cypress - all pass locally on my system.)

Copy link
Member

@line-o line-o left a comment

Choose a reason for hiding this comment

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

These changes are a big step forward for new users to understand the URL rewriting.
Many thanks.
I found some typos and have some questions. These can all be addressed at a later point if you do not feel they are urgent.

src/main/xar-resources/data/urlrewrite/urlrewrite.xml Outdated Show resolved Hide resolved
src/main/xar-resources/data/urlrewrite/urlrewrite.xml Outdated Show resolved Hide resolved
src/main/xar-resources/data/urlrewrite/urlrewrite.xml Outdated Show resolved Hide resolved
src/main/xar-resources/data/urlrewrite/urlrewrite.xml Outdated Show resolved Hide resolved
src/main/xar-resources/data/urlrewrite/urlrewrite.xml Outdated Show resolved Hide resolved
src/main/xar-resources/data/urlrewrite/urlrewrite.xml Outdated Show resolved Hide resolved
src/main/xar-resources/data/urlrewrite/urlrewrite.xml Outdated Show resolved Hide resolved
@joewiz joewiz self-assigned this Oct 3, 2023
@duncdrum
Copy link
Contributor

@joewiz does this need a rebase, or have these changes already been incorporated ?

… most other articles in documentation use 2-space indent
@joewiz
Copy link
Member Author

joewiz commented Dec 19, 2023

@duncdrum It needs a rebase. Coming...

@joewiz joewiz force-pushed the url-rewrite-edits branch 2 times, most recently from ccf9e0b to 633fbe6 Compare December 19, 2023 18:27
Copy link
Contributor

@duncdrum duncdrum left a comment

Choose a reason for hiding this comment

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

Nice improvements. @line-o comments are still unresolved. I added a few stylistic notes. We should be consistent in the use of $ when referring to Xquery variable names.

@duncdrum
Copy link
Contributor

@joewiz could you have a look at @line-o and my comments, and see which ones you can implement and which require more input?

@joewiz
Copy link
Member Author

joewiz commented Dec 20, 2023

@line-o @duncdrum Thanks for your comments! I've incorporated them in my most recent commit.

Copy link
Contributor

@duncdrum duncdrum left a comment

Choose a reason for hiding this comment

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

@line-o lets tackle #940 in a separate PR.
@joewiz one last suggestion for you to accept or not. Otherwise this is good to merge.

@duncdrum duncdrum merged commit 430bc37 into eXist-db:master Dec 20, 2023
2 checks passed
@joewiz joewiz deleted the url-rewrite-edits branch December 20, 2023 23:59
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.

4 participants