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

Unused Stapler Facet is removed #121

Merged
merged 5 commits into from
Nov 21, 2022

Conversation

jgreffe
Copy link
Contributor

@jgreffe jgreffe commented Nov 2, 2022

Fixes #87

Remove all classes related to Facet
Remove all plugin.xml declarations related to Facet
Fixes infinite loop in JexlInspection.tokenize("'") : case ${%Test('
Fixes out of range issue with patterns like ${%Test(something)} (Argument rangeInElement (20,21) endOffset must not exceed descriptor text range (121, 141) length (20))

Upgrading to this new plugin version on existing projects having facet will popup a warning message:
Monosnap Error 2022-11-02 12-50-04

  • 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 duemir and timja November 3, 2022 01:00
@duemir
Copy link
Member

duemir commented Nov 3, 2022

@timja I am on the fence about this. What do you think?

@timja
Copy link
Member

timja commented Nov 3, 2022

Sounds fine to remove if it’s not being used, but not too familiar with this

@duemir duemir mentioned this pull request Nov 9, 2022
@duemir
Copy link
Member

duemir commented Nov 14, 2022

OK. We should probably do it. It is unlikely that this Facet was ever used in the field by anybody but me.
But I think we should add a few more tests for JexlInspection, which is being published to a wider audience by this change. e.g., test that it handles property lookups (l10n) like ${%Name} and ${%Build Queue(items.size())}

@jgreffe
Copy link
Contributor Author

jgreffe commented Nov 16, 2022

Added some unit tests, and fixed two issues:

  • infinite loop in JexlInspection.tokenize("'")
  • out of range issue with patterns like ${%Test(something)} (Argument rangeInElement (20,21) endOffset must not exceed descriptor text range (121, 141) length (20))

Copy link
Member

@duemir duemir left a comment

Choose a reason for hiding this comment

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

A few minor suggestions.

I've confirmed the test fail and that changes in Jexl Inspection fix them. I have a hard time reasoning about this thing, so this is as best I can do in a limited time.

@jgreffe jgreffe requested review from duemir and removed request for timja November 18, 2022 08:16
@duemir duemir changed the title Is Stapler Facet even needed? Unused Stapler Facet is removed Nov 21, 2022
@duemir duemir merged commit 7ff60ed into jenkinsci:master Nov 21, 2022
@duemir duemir added internal Pull requests that update anything related to building the plugin but not the plugin itself bump-minor Tell release drafter to increase Minor version labels Nov 21, 2022
@duemir
Copy link
Member

duemir commented Nov 22, 2022

Oh boy! I built a local version of the next release. Ran the JEXL inspection on @jenkinsci/jenkins . It crashes in two files and shows 11 errors in 9 files.

4 has to do with localization. The rest is about HTML-encoded angle bracket 🤔

@jgreffe
Copy link
Contributor Author

jgreffe commented Nov 23, 2022

@duemir : this means as jexl inspection is now activated by default, it displays proper expected errors, but also points out some faulty plugin code (crashes) ?

@duemir
Copy link
Member

duemir commented Nov 24, 2022

@jgreffe Yes, as far as I can see. I will try to iron at least some out.

@jgreffe
Copy link
Contributor Author

jgreffe commented Nov 24, 2022

Crashes occur with i18n like:

<j:jelly xmlns:j="jelly:core">
    <p>
        ${%Te'st}</p>
</j:jelly>

@duemir duemir added bump-major Tell release drafter to increase Major version major-enhancement and removed bump-minor Tell release drafter to increase Minor version internal Pull requests that update anything related to building the plugin but not the plugin itself labels Mar 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump-major Tell release drafter to increase Major version major-enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is Stapler Facet even needed?
3 participants