Tried to file this under an icon-shim-plugin component but could not find one in JIRA.

      At any rate, IconSet has overloads taking Groovy GString. This kind of thing should not appear in an API. Take only a String, or if you must, accept an Object and call toString (if not null).

          [JENKINS-25498] Inappropriate use of GString

          Tom FENNELLY added a comment -

          Yeah, this is horrible and would be more than happy to get rid of it if possible. Please advise.

          KK was asking me about this some time ago too. Here's the conversation...

          I'll check is it still needed. At the time I added it though it was definitely needed - I didn't add it for fun.

          From memory, I think it was down to JEXL (I may be wrong - it was a while ago). I think it was needed in situations where the 'abc' variable you talk about in your example was the output of an earlier JEXL expression that created a GString object instance in the jelly context used to evaluate the expressions. Then later, when ${icons.getIconByUrl(abc)} is being evaluated, JEXL was trying to find an Icons.getIconByUrl(GString) method (because abc is of type GString) but was failing.

          As I say... this is from memory so I may be slightly wrong on the exact details, but I don't think I'm too far off and I certainly did not add it randomly. That said, it may be that this is no longer needed because I do remember having some trouble implementing the tag library in that plugin, which resulted in me implementing it a few different ways before I was able to get it to work (http://goo.gl/RSbacS). So maybe the GStrings are legacy from one of those attempts.

          On 19/10/2014 06:01, Kohsuke Kawaguchi wrote:
          > I remember what I wanted to ask you.
          >
          > In the following code, you have every method overloaded between String and GString.
          >
          > https://github.com/jenkinsci/icon-shim-plugin/blob/master/icon-set/src/main/java/org/jenkins/ui/icon/IconSet.java
          >
          > What is that for? GString is mostly internal implementation details in Groovy, and it'll perform the appropriate type coercison to generate a plain String. So this seems redundant.
          >
          > And worse yet, JEXL (used by Jelly) has a trouble resolving overload when the null is an argument. So for example, invocations like
          >
          > ${icons.getIconByUrl(abc)}
          >
          > results in a problem if 'abc' is null.
          >
          > –
          > Kohsuke Kawaguchi

          Tom FENNELLY added a comment - Yeah, this is horrible and would be more than happy to get rid of it if possible. Please advise. KK was asking me about this some time ago too. Here's the conversation... I'll check is it still needed. At the time I added it though it was definitely needed - I didn't add it for fun. From memory, I think it was down to JEXL (I may be wrong - it was a while ago). I think it was needed in situations where the 'abc' variable you talk about in your example was the output of an earlier JEXL expression that created a GString object instance in the jelly context used to evaluate the expressions. Then later, when ${icons.getIconByUrl(abc)} is being evaluated, JEXL was trying to find an Icons.getIconByUrl(GString) method (because abc is of type GString) but was failing. As I say... this is from memory so I may be slightly wrong on the exact details, but I don't think I'm too far off and I certainly did not add it randomly. That said, it may be that this is no longer needed because I do remember having some trouble implementing the tag library in that plugin, which resulted in me implementing it a few different ways before I was able to get it to work ( http://goo.gl/RSbacS ). So maybe the GStrings are legacy from one of those attempts. On 19/10/2014 06:01, Kohsuke Kawaguchi wrote: > I remember what I wanted to ask you. > > In the following code, you have every method overloaded between String and GString. > > https://github.com/jenkinsci/icon-shim-plugin/blob/master/icon-set/src/main/java/org/jenkins/ui/icon/IconSet.java > > What is that for? GString is mostly internal implementation details in Groovy, and it'll perform the appropriate type coercison to generate a plain String. So this seems redundant. > > And worse yet, JEXL (used by Jelly) has a trouble resolving overload when the null is an argument. So for example, invocations like > > ${icons.getIconByUrl(abc)} > > results in a problem if 'abc' is null. > > – > Kohsuke Kawaguchi

          Tom FENNELLY added a comment -

          True, accepting an Object would be a bit better than GString if it worked.

          Tom FENNELLY added a comment - True, accepting an Object would be a bit better than GString if it worked.

          Jesse Glick added a comment -

          Yes please dig up the original justification for this because it seems wrong. It also sounds like KK’s warning anticipated JENKINS-25499 exactly.

          Jesse Glick added a comment - Yes please dig up the original justification for this because it seems wrong. It also sounds like KK’s warning anticipated JENKINS-25499 exactly.

          Daniel Beck added a comment - - edited

          Tom: I have a chatlog around somewhere where I asked you about that, maybe that has more detail.

          Update: Unfortunately not.

          [2014-08-30T01:19:33+0200] <+danielbeck> https://github.com/jenkinsci/icon-shim-plugin/blob/master/icon-set/src/main/java/org/jenkins/ui/icon/IconSet.java#L88 GString?
          [2014-08-30T01:20:25+0200] <+danielbeck> What do you need Groovy strings for?
          [2014-08-30T01:20:40+0200] <tfennelly> danielbeck: trying to remember
          [2014-08-30T01:21:01+0200] <tfennelly> danielbeck: oh I think it was wrt jexl
          [2014-08-30T01:21:38+0200] <tfennelly> danielbeck: forget exactly but it's needed sometimes... otherwise not resolved

          Daniel Beck added a comment - - edited Tom: I have a chatlog around somewhere where I asked you about that, maybe that has more detail. Update: Unfortunately not. [2014-08-30T01:19:33+0200] <+danielbeck> https://github.com/jenkinsci/icon-shim-plugin/blob/master/icon-set/src/main/java/org/jenkins/ui/icon/IconSet.java#L88 GString? [2014-08-30T01:20:25+0200] <+danielbeck> What do you need Groovy strings for? [2014-08-30T01:20:40+0200] <tfennelly> danielbeck: trying to remember [2014-08-30T01:21:01+0200] <tfennelly> danielbeck: oh I think it was wrt jexl [2014-08-30T01:21:38+0200] <tfennelly> danielbeck: forget exactly but it's needed sometimes... otherwise not resolved

          Tom FENNELLY added a comment -

          Fixed this issue (and JENKINS-25499) in the icon-shim plugin. Updated dependency to new version of same.

          Tom FENNELLY added a comment - Fixed this issue (and JENKINS-25499 ) in the icon-shim plugin. Updated dependency to new version of same.

            tfennelly Tom FENNELLY
            jglick Jesse Glick
            Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

              Created:
              Updated:
              Resolved: