Should BaseProcessor default to None for string-returning methods? #156

Open
relrod opened this Issue Jul 20, 2013 · 4 comments

Projects

None yet

2 participants

@relrod
Fedora Infrastructure member

In meta.base.BaseProcessor, there are some methods (like link()) which return strings, but they default to "" - this makes serializing as e.g., JSON ugly.

For example, in my HRF/Mobile combination, because of this default, HRF returns {"link": ""}.

Mobile parses this into a Scala case class which has link: Option[String] -- Option is an Option type.

In short, it's a way to hold and act on a value, if it exists, otherwise it'll be None. I use this for link because it might not exist, but since the default is "", the Option doesn't return None, it returns Some("") -- when we map over that to enable clicking on a row in the newsfeed (to view the link in the browser) (see these lines), it tries to spawn an activity based on the protocol of the URL (usually http or https, which launches a browser), it looks for an activity that responds to the url "" and comes up empty handed, which fatals the app.

There's a few solutions here.

1) I can stop using Option[String] for Link, but this seems odd since link really is optional.
2) I can check that it's not "" but that seems .. pointless.
3) I can handle the exception - maybe I should do this anyway, but that's a band-aid.
4) I can fix this in HRF - check for "" and change it to None (which serializes to null)
5) (my preferred) We can fix it here in fedmsg, by making these methods return None, which seems like the most reusable case that would benefit everyone who does any form of serialization.

Thoughts?

@ralphbean

Food for thought: check out this "legacy_condition" decorator in fedmsg.meta. https://github.com/fedora-infra/fedmsg/blob/develop/fedmsg/meta/__init__.py#L116

@ralphbean

Solution number 5 is the way to go, for sure. We just have to watch where that may or may not trip up other code that is expecting strings. fedmsg-irc for instance... but also everywhere else. I'm all for making the change; we just have to be ready to deal with breakage elsewhere.

@relrod
Fedora Infrastructure member

In actuality, I didn't think about this earlier but we should probably do this for all optional methods in meta, not just the string ones.

I know this only increases your concern about breaking other things, though. :(

How do you think we should proceed on this? Do you have a list of things in mind that use meta and would need to be changed?

@ralphbean

Let's define it a little more clearly.

  • If there's a reasonable meta value to be returned, it should be.
  • If that reasonable value really is an empty object, (say an empty set() or an empty string "", so be it).
  • If there is no reasonable value to be returned, the return value should be None.

So, in the case you bring up with msg2link, the return value should be None because there really is no link.

Take another case with msg2usernames; it should just return the empty set set() for some messages because there really are 0 users associated with the messages. It's not that its undefined (i.e., None), but that there are definitely these 0 users to be returned.


Here's a quick list of places where we'll need to check for breakage on update of msg2link:

  • fedmsg-irc
  • fedmsg-tweet
  • fedmsg-tail
  • datanommer
  • datagrepper
  • fedmsg-notify
  • fedbadges
  • FMN
  • maybe busmon?
  • maybe fedmsg-middleware? (although thats not deployed or ready anywhere)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment