[
https://issues.apache.org/jira/browse/SHINDIG-124?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12580620#action_12580620
]
Michael Papp commented on SHINDIG-124:
--------------------------------------
Hope you don't mind, Cassie - I added your list comments to the ticket below:
=======================================================================
---------- Forwarded message ----------
From: Cassie <[EMAIL PROTECTED]>
To: [email protected]
Date: Wed, 19 Mar 2008 09:54:34 +0100
Subject: Re: [jira] Commented: (SHINDIG-124) Implementation POJO for
opensocial.Enum.xx and opensocial.Message
Taking off the bug because this has turned into a longer thing..
On Tue, Mar 18, 2008 at 7:03 PM, Michael Papp (JIRA) <[EMAIL PROTECTED]>
wrote:
>
> [
> https://issues.apache.org/jira/browse/SHINDIG-124?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12579963#action_12579963]
>
> Michael Papp commented on SHINDIG-124:
> --------------------------------------
>
> Hi Cassie - thanks again for your assistance with this code.
>
> 1. Will remove author tag
>
> 2. Here is my reasoning for the implementation:
> a. Rules for Enum:
> displayValue can never be null.
> Key may be null.
> if displayValue is one of the pre-defined constants, then
> displayValue == Key.toString().
I think this assumption is where things break. We shouldn't assume it will
be key.toString() it should be allowed to be an arbitrary string as well. If
we want to include a helper constructor that just takes in the enum only and
uses enum.toString for the displayValue then that is fine. it only needs to
be a helper though.
>
> if displayValue is not one of the pre-defined constants,
> then Key = null; displayValue = arbitrary string.
> b. Enum(Key key, String displayValue) is an unnecessary constructor, as
> either the Key is equivalent to the Value (String), or the Key != Value and
> the Key is null. Therefore, only the displayValue matters - not the Key.
> Still, for conformity, I could implement this constructor and throw away
> the Key.
if we go this route, you should really throw away the displayValue... not
the key. then you wouldn't need all of the lookup logic. however, as i
mentioned above, i think both should be preserved in this constructor. so
perhaps this:
public Enum(String displayValue) {
this(null, displayValue)
}
public Enum(T key) {
this(key, key.toString())
}
public Enum(T key, String displayValue) {
// set both
}
>
> c. The reverse look up on the enum Key properties enforces this
> contract.
Ah, so perhaps I simply conveyed the contract incorrectly. Here is what we
are bound to:
- The displayValue can be any String in the world
- The key must be one of the predefined enums, or null
Thus, the displayValue and the key have no relation to each other (spec wise
that is). Now, I'll give you a case where this makes sense. In Orkut, let's
say they have some radio buttons for your smoker value that look like this:
"unhip, rock it always, when i feel like it"
Which would translate into "no, yes, occasionally" (or whatever). Now, orkut
would probably use their container specific displayValues so that when
gadgets are showing information to the user the user knows what it is
talking about (in relation to the container) Thankfully, the gadgets also
have the keys though which means they know the Orkut's "unhip" is the same
as myspace's "no" without having a bunch of container specific logic in
their app.
>
> As per your previous response and my understanding of the API, this
> implementation fits the requirements (and darn it, I am a stickler for
> requirements). And while I can see the motivation for providing placeholder
> model classes and let developers fill in the blanks, I generally believe
> that such 'model' implementations should serve as reference implementations
> (go the extra mile, so to speak). All that said, I really do like your
> generics approach, much cleaner looking, etc. Let me play around with this
> today and respond with something this afternoon. Unless you or the team
> think the "contract enforcement" aspect is overboard, I really do urge the
> interested parties to keep this in the implementation.
I completely agree with your contract enforcement approach, I just think
that it doesn't exactly match the contract that the spec wanted to put out
their. Which would be a burden on containers.
So essentially, your coding styles are now aligned with Shindig, but I think
the concept behind it is still a little off. Again, I know this is super
complicated, and it is entirely my fault for not explaining it well enough.
Thank you for all the time you are spending on this!
- Cassie
===============================================================================
OK, I am almost done with the Generics approach and it is *indeed* looking
nice. I also want to thank you for putting up with my slow uptake on the
contract semantics inherent in the spec. I guess it is a matter of coming from
the world of operating systems and app frameworks where the spec is cast in
concrete and you can modify the behavior in your own code. This spec is a bit
more like "here are the calls, implement them as fits your needs." That is
fine, as long as everyone tries to remain consistent. The use case that
bothers me is when the Key is set to "NO" and the displayValue is set to "yes".
The developer (and end user) will obviously get different answers dependent on
which field is queried, and logic which depends on a consistent behavior will
not work. Of course it behooves the implementor to "make it work" by keeping
the two in some sort of sync. OTOH, a standard reference semantic behavior
helps avoid those sorts of mistakes. Skimming the various lists on OpenSocial
quickly turns up developers complaints about fragmentation and/or a standard
without common semantics. That said, I would strongly recommend giving a some
formal semantics to the Shindig implementation, and letting developers change
the behavior in the .js or Java layers by subclassing or modifying code. OK,
soapbox off.
I will have a new patch for you tomorrow, very clean and simple. Thank you for
the Generics idea - quite brilliant.
> Implementation POJO for opensocial.Enum.xx and opensocial.Message
> -----------------------------------------------------------------
>
> Key: SHINDIG-124
> URL: https://issues.apache.org/jira/browse/SHINDIG-124
> Project: Shindig
> Issue Type: New Feature
> Components: Gadgets Server - Java
> Reporter: Michael Papp
> Attachments: fix-124-1-bug.patch.txt, fix-124-2-bug.patch.txt,
> fix-124-bug.patch.txt
>
>
> Submitting proposed implementation for opensocial.Enum and
> opensocial.Enum.Drinker, opensocial.Enum.Gender, and opensocial.Enum.Smoker.
> This is a best guess pass as there are a number of ways of implementing this,
> and opensocial 0.7 spec for the getKey() is rather nebulous. Anyway, hope
> this helps. If approach is right, then I can amend patch to include tests.
> M. Papp
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
