If we are only going to allow strings in the map, then we don't need a new
method.
If we are going to allow other values in the map, then we would need a new
method.
Maybe I lost the plot, but I don't see the point of allowing people to put
non-string objects into the map unless they can get non-string objects back.
On Thu, Mar 25, 2021 at 10:51 AM Ralph Goers <ralph.go...@dslextreme.com>
wrote:
> The existing get method does a deep toString on the value object already.
> I’m not sure why we would need a new method if the return value is a String.
>
> Ralph
>
> > On Mar 25, 2021, at 10:28 AM, Tim Perry <tim.v...@gmail.com> wrote:
> >
> > Would we also have
> > public String getObject(String) {...}
> >
> > What will happen when someone calls get and the object that key is mapped
> > to is not a String? Does it return null? Throw an exception?
> >
> > Instead of allowing non-string values maybe change the type of data to
> > Map<String, String> instead of <String, Object> and avoid the mess?
> >
> >
> > On Thu, Mar 25, 2021 at 8:02 AM Gary Gregory <garydgreg...@gmail.com>
> wrote:
> >
> >> The 2 postfix gives me flashbacks of Microsoft COM interfaces! :-p
> >> putObject is less shudder inducing than put2... for me at least.
> >>
> >> Gary
> >>
> >>
> >> On Thu, Mar 25, 2021, 01:08 Remko Popma <remko.po...@gmail.com> wrote:
> >>
> >>> Haha!
> >>> putObject?
> >>>
> >>>
> >>>
> >>>> On Mar 25, 2021, at 11:39, Ralph Goers <ralph.go...@dslextreme.com>
> >>> wrote:
> >>>>
> >>>> I’m sure that will drive Gary nuts. Let’s call the new method
> >> “put2()”.
> >>>>
> >>>> Ralph
> >>>>
> >>>>> On Mar 24, 2021, at 5:18 PM, Remko Popma <remko.po...@gmail.com>
> >> wrote:
> >>>>>
> >>>>> Instead of overloading the existing method(s), how about adding new
> >>> methods
> >>>>> with a slightly different name that takes Object parameters?
> >>>>>
> >>>>>> On Thu, Mar 25, 2021 at 8:46 AM Carter Kozak <cko...@ckozak.net>
> >>> wrote:
> >>>>>>
> >>>>>> The method argument type changes are an ABI break, but I recall
> >>> extending
> >>>>>> MapMessage within a project in order to support more expressive
> types
> >>> --
> >>>>>> that
> >>>>>> may have relied on dangerously casting the result of
> >>>>>> getIndexedReadOnlyStringMap
> >>>>>> to an IndexedStringMap.
> >>>>>> Including a safer Object-valued MapMessage subclass (with overloaded
> >>> put
> >>>>>> methods)
> >>>>>> sounds reasonable to me given at least two of us have run into this!
> >>>>>>
> >>>>>> -Carter
> >>>>>>
> >>>>>> On Wed, Mar 24, 2021, at 19:29, Remko Popma wrote:
> >>>>>>> I called it StringMap because the keys must be Strings. Admittedly
> >>> not a
> >>>>>>> great name. :-)
> >>>>>>>
> >>>>>>> Not sure exactly, but there may be cases where this change could
> >>> cause an
> >>>>>>> issue:
> >>>>>>> putAll(final Map<String, String> map) ->
> >>>>>>> putAll(final Map<String, Object> map)
> >>>>>>>
> >>>>>>> On Thu, Mar 25, 2021 at 2:12 AM Ralph Goers <
> >>> ralph.go...@dslextreme.com
> >>>>>> <mailto:ralph.goers%40dslextreme.com>>
> >>>>>>> wrote:
> >>>>>>>
> >>>>>>>> I looked the other day and wondered the same thing Volkan did.
> >> There
> >>>>>> are
> >>>>>>>> no unit tests and the contributor didn’t even indicate that he had
> >>>>>> tried
> >>>>>>>> it.
> >>>>>>>>
> >>>>>>>> I was initially concerned that the underlying Map wouldn’t support
> >> it
> >>>>>>>> since it has StringMap in its name. It turns out the values are
> >>>>>> objects.
> >>>>>>>>
> >>>>>>>> Technically I don’t think this would break compatibility. Any code
> >>>>>>>> referencing the put(String, String) would automatically map to
> >>>>>> put(String,
> >>>>>>>> Object). He didn’t modify the get method which would have broken
> >>>>>>>> compatibility.
> >>>>>>>>
> >>>>>>>> Ralph
> >>>>>>>>
> >>>>>>>>> On Mar 24, 2021, at 8:27 AM, Matt Sicker <boa...@gmail.com
> >> <mailto:
> >>>>>> boards%40gmail.com>> wrote:
> >>>>>>>>>
> >>>>>>>>> Pretty sure that would break binary compatibility since it
> removes
> >>>>>> the
> >>>>>>>>> String method. I think it might be addable but not removed like
> >>> that.
> >>>>>>>>>
> >>>>>>>>> On Wed, 24 Mar 2021 at 02:39, Volkan Yazıcı <
> >>> volkan.yaz...@gmail.com
> >>>>>> <mailto:volkan.yazici%40gmail.com>>
> >>>>>>>> wrote:
> >>>>>>>>>>
> >>>>>>>>>> Hello,
> >>>>>>>>>>
> >>>>>>>>>> Adding non-String-typed value support to MapMessage was also
> >>>>>> something
> >>>>>>>> on
> >>>>>>>>>> my radar too. But this PR replacing String with Object in two
> >> lines
> >>>>>>>> seems
> >>>>>>>>>> too good to be true to me. Does anybody mind taking a second
> look
> >>> at
> >>>>>>>> this,
> >>>>>>>>>> please?
> >>>>>>>>>>
> >>>>>>>>>> Kind regards.
> >>>>>>>>>>
> >>>>>>>>>> ---------- Forwarded message ---------
> >>>>>>>>>> From: Henry Widd <notificati...@github.com <mailto:
> >>>>>> notifications%40github.com>>
> >>>>>>>>>> Date: Tue, Mar 23, 2021 at 4:58 PM
> >>>>>>>>>> Subject: [apache/logging-log4j2] MapMessage put methods should
> >> not
> >>>>>>>> mandate
> >>>>>>>>>> String values (#477)
> >>>>>>>>>> To: apache/logging-log4j2 <logging-log...@noreply.github.com
> >>>>>> <mailto:logging-log4j2%40noreply.github.com>>
> >>>>>>>>>> Cc: Subscribed <subscri...@noreply.github.com <mailto:
> >>>>>> subscribed%40noreply.github.com>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> the underlying Map is typed <String,Object> so the put methods
> on
> >>>>>>>>>> MapMessage can also be.
> >>>>>>>>>> ------------------------------
> >>>>>>>>>> You can view, comment on, or merge this pull request online at:
> >>>>>>>>>>
> >>>>>>>>>> https://github.com/apache/logging-log4j2/pull/477
> >>>>>>>>>> Commit Summary
> >>>>>>>>>>
> >>>>>>>>>> - MapMessage put methods should not mandate String values
> >>>>>>>>>>
> >>>>>>>>>> File Changes
> >>>>>>>>>>
> >>>>>>>>>> - *M*
> >>>>>>>>>>
> >>>>>>>>
> >>>>>>
> >>>
> log4j-api/src/main/java/org/apache/logging/log4j/message/MapMessage.java
> >>>>>>>>>> <
> >>>>>>>>
> >>>>>>
> >>>
> >>
> https://github.com/apache/logging-log4j2/pull/477/files#diff-f03ffe9ceefd37c87fd118ce591bd8ad288e43b08cd663dde14441f4e7c117ef
> >>>>>>>>>
> >>>>>>>>>> (6)
> >>>>>>>>>>
> >>>>>>>>>> Patch Links:
> >>>>>>>>>>
> >>>>>>>>>> - https://github.com/apache/logging-log4j2/pull/477.patch
> >>>>>>>>>> - https://github.com/apache/logging-log4j2/pull/477.diff
> >>>>>>>>>>
> >>>>>>>>>> —
> >>>>>>>>>> You are receiving this because you are subscribed to this
> thread.
> >>>>>>>>>> Reply to this email directly, view it on GitHub
> >>>>>>>>>> <https://github.com/apache/logging-log4j2/pull/477>, or
> >>> unsubscribe
> >>>>>>>>>> <
> >>>>>>>>
> >>>>>>
> >>>
> >>
> https://github.com/notifications/unsubscribe-auth/AAARTSKGBRHC4NG637EHA4LTFC3BTANCNFSM4ZVO7L2Q
> >>>>>>>>>
> >>>>>>>>>> .
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>
> >>>>>>
> >>>>
> >>>>
> >>>
> >>
>
>
>
