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

jellytag extension is now supported since Stapler supports it for Tag Libraries. #97

Merged

Conversation

duemir
Copy link
Member

@duemir duemir commented Jun 30, 2022

While investigating the StackOverflowError-s I noticed that XML metadata contributed by the plugins is specific for the CustomTagLibrary feature of Stapler. I decided to:

  • Rename the classes appropriately to better align with stapler's codebase and hopefully improve readability
  • Add the "jellytag" to the supported Jelly file extensions since it is expected by at least two tag library implementations: CustomTagLibrary and ThisTagLibrary. (It is barely used in real life, though).
  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@duemir duemir requested review from oleg-nenashev and timja June 30, 2022 13:52
@duemir duemir self-assigned this Jun 30, 2022
timja
timja previously approved these changes Jun 30, 2022
Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

lgtm although barely used, (I've never seen it used although I had seen it in stapler before)

Comment on lines 31 to 32
* Stapler's <a href="https://github.com/jenkinsci/stapler/blob/1709.ve4c10835694b_/jelly/src/main/java/org/kohsuke/stapler/jelly/CustomTagLibrary.java#L128-L130">CustomTagLibrary</a> supports standard Jelly Extension and also introduces its own tag extension.
* It is also extensible using {@code JellyTagFileLoader}-s, the only instance of which is <a href="https://github.com/jenkinsci/stapler/blob/2a13b906bf3af42bc610e4592d56eb8b511fa1be/groovy/src/main/java/org/kohsuke/stapler/jelly/groovy/GroovyTagFileLoader.java">GroovyTagFileLoader</a>
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. It is a private field so it shouldn't be included in JavaDoc anyway. I'll just turn it into a regular multiline comment.

@duemir duemir closed this Jul 12, 2022
@duemir duemir reopened this Jul 12, 2022
@duemir duemir merged commit cd35e3b into jenkinsci:master Jul 12, 2022
@duemir duemir deleted the 66-stack-overflow-from-jelly-xml-metadata branch July 13, 2022 05:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants