Move SiriusFactory to api package (per issue #11)

[smuir]

As per #11, the SiriusFactory class, as the only way to create an instance of Sirius, is really part of the API, and hence should be in the api package, not impl.

[comcast-jonm]

I think we have to be careful about this. If SiriusFactory is part of the public API then changing its package breaks backwards compatibility.

[clinedome-work]

This change is not backwards-compatible. How are we planning to manage such things? With it should probably come an explicit major-version revision bump (2.0.0-SNAPSHOT). Should we perhaps use a branch on Comcast/sirius for applying these kinds of changes?

[smuir]

Presumably SiriusFactory must be part of the public API if it is the only (or recommended) way to create instances.

As for backwards compatibility: understood, but if we agree that SiriusFactory is in the wrong place (and isn't part of the point of having api and impl packages being to separate what's logically exposed to consumers and what's not?) then it's better to make such a change sooner, before we have real external users.

[comcast-jonm]

So: I agree with the value of changing the package. You're right; it's in the wrong place.

I also think it is important to figure out how to address things like this without making backwards-incompatible changes, because it will be in our (and our users') best interests to go for as long as we can without a major version change.

As I look at the current implementation, I see that the SiriusFactory returns a SiriusImpl from its createInstance method; this probably ought to just be a Sirius. Therefore, I propose the following approach:

1. Create a copy of SiriusFactory in the desired package, with the type signature change of returning an interface rather than a concrete class.
2. Mark the existing version as deprecated; refactor it to call the 'real' implementation in the correct location.

It does create some code duplication but it's mostly interface/adapter boilerplate. I'd be ok with that.

[smuir]

That sounds like a reasonable suggestion. I somehow didn't consider the backwards compatibility implications in making this change hastily. I'm not personally a huge fan of two classes with the same name, even if one just wraps the other, and I'm not sure deprecation ever really has much effect (how many deprecated methods from Java 1.1 are still in the Java 7 API?), but I like this approach. We could name the new class something other than SiriusFactory, but I don't have any great suggestions (SiriusBuilder?).

While we're on the topic of moving stuff around then, does it even make sense to have an explicitly named API package? That seems unusual. Given the lengthy com.comcast.xfinity.sirius prefix (do we need the xfinity part?), any way to save 4 characters is helpful. However, I realise that major changes like that are likely to be very messy with little practical impact.

[clinedome-work]

Even if we have to have two of them, one deprecated, I'd rather keep the naming consistent. Let's call it what it is. And I don't think it's such a big deal to have deprecated things -- bytes for the extra classes don't really cost anything, and if we give folks a good pointer from the deprecated to the non-deprecated version of something, they'll take the advice.

[comcast-jonm]

Right. The purpose of deprecation is really twofold:

1. Give current users a heads-up so they have the opportunity to migrate off at their convenience. If this is done well, then a version 2.0 transition should be easier.
2. Give us a way to track things we can get rid of when we do finally roll a 2.0.

---

From: Peter Cline [notifications@github.com]
Sent: Thursday, February 13, 2014 9:24 AM
To: Comcast/sirius
Cc: Moore, Jonathan (T+P)
Subject: Re: [sirius] Move SiriusFactory to api package (per issue #11) (#23)

Even if we have to have two of them, one deprecated, I'd rather keep the naming consistent. Let's call it what it is. And I don't think it's such a big deal to have deprecated things -- bytes for the extra classes don't really cost anything, and if we give folks a good pointer from the deprecated to the non-deprecated version of something, they'll take the advice.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/23#issuecomment-34982188.

[smuir]

I guess we'll see how well deprecation works when (if) we do cut a 2.0 release. I'm not arguing against taking the approach suggested by Jon though, so I'll make a less intrusive that just adds a new wrapper class. Is there a preference for committing a new change that moves the existing file back to its original location vs abandoning this PR and just creating a small patch that adds the new wrapper?

[smuir]

Any thoughts on the need for an API directory at all? I note that the current top-level Sirius directory is currently empty, so there would no pollution of that namespace.

[clinedome-work]

I think SiriusFactory could live in the sirius package.

I'd suggest modifying this PR. Commit new things on top of it, and then you can rebase/squash/etc it down to the right logical commits (either sooner or later).

[smuir]

okay, will do. I like that as a good starting point.

[smuir]

Okay, so I think the PR actually looks reasonable now. A roundabout way of getting to the right point, but it seems like the final patch (add SiriusFactory to the top-level sirius directory) is correct.

[comcast-jonm]

Yes, I like this direction. Couple more minor suggestions:

• Can we drop a deprecated annotation on the old SiriusFactory?
• I'd vote for having the old version delegate to the new version (i.e. move the logic into the new version) instead, as this underscores that the old version exists only for backwards-compatibility interface reasons and is not the locus of any interesting functionality for ongoing development.

[clinedome-work]

Also need to revamp this a little. Sirius trait doesn't have shutdown(), for one, and we're return Sirius instead of SiriusImpl.

[clinedome-work]

We might need to add shutdown() to the Sirius trait, if we think anybody's using it. They probably are. We certainly use it in tests. Same for onShutdown(), and perhaps for getStatus and getMembership... unless we add those to something like Jon's Sirius1Dot2Extensions.

[smuir]

I thought about the second point before doing it the way I did, I think I might have convinced myself that this was better but can't remember why. Ah, now I remember: if the return type of the new method is Sirius (as you suggested, which I like) vs SiriusImpl then the deprecated method would need to cast a Sirius back to SiriusImpl, which is practically okay but stylistically ugly. I see Peter made some comments about the return type though, so I'll look at that.

[mbyron01]

So let me see if I get all this straight. The new version of SiriusFactory will be moved to the sirius package. We deprecate the old SiriusFactory modifying it to call the new SiriusFactory and having it still return a SiriusImpl. The new SiriusFactory however will return a Sirius trait/interface type.

[comcast-jonm]

@mbyron01 Well, that's the current proposal. I actually think the SiriusFactory should return a Sirius and not a SiriusImpl; that's kind of the point of the Factory pattern. Yes, this involves an ugly cast in the old factory, but it's pretty minor and easy to unit test.

If we want to expose the other methods on the existing SiriusImpl then they should be added to the 1.2 extensions trait, and the new SiriusFactory should have a:

object SiriusFactory {
  def create1Dot2Instance(...): Sirius1Dot2 = { ... }
}

We want to get to the point where people are depending pretty much just on:

• SiriusFactory
• SiriusConfiguration
• Sirius or Sirius1Dot2
• RequestHandler or RequestHandler1Dot2

If those are all in the sirius package and not much else is, then we can be pretty clear about what things we're guaranteeing backwards compatibility on.

[smuir]

Maulan, I think that's correct, as long as everyone is okay with an ugly cast in the deprecated method. I guess since it's deprecated it's not a big deal.

[smuir]

Jon, that sounds like another argument for creating your Sirius1Dot2 trait be independent of the initialisation callback changes.

[clinedome-work]

@comcast-jonm I think we're probably pretty free to add to the Sirius trait, if we like. Since we've been returning SiriusImpls, it's not like anyone's been using the trait -- and adding is pretty safe anyway. I think it's more a question of how we want that trait to look, and how we pave the way for people transition smoothly when they switch to the new version. FWIW, since we've been returning the implementation, the list of public methods on SiriusImpl has been our de facto public interface.

[comcast-jonm]

Adding methods to an interface and adding methods to a class/object are distinctly different from a backwards-compatibility point of view. In particular, if someone did implement the Sirius trait (perhaps to decorate a SiriusImpl) then adding methods to the Sirius trait breaks compatibility since all implementors--including those we may not know about--have to implement those methods too, which breaks the semantic versioning contract for minor versions. Ditto abstract methods on classes.

[clinedome-work]

Definitely true. But we are in the unique situation where we're in pre-public mode and know all of our clients. If we decided that the Sirius trait itself should look a little different before release, we could probably get away with that.

We haven't been great about SemVer thus far, but I guess we're public as of 1.1.3 -- so yeah, we should try be compliant starting there.

[smuir]

Okay, I think it won the fight with git (thanks Jon for the reminder to use the --force flag), this looks like a decent pull request. It doesn't address all the questions about the Sirius trait, but it puts the SiriusFactory in the correct place, and updates the legacy class and associated test to point to the new class.

[comcast-jonm]

@smuir: Can you resolve the merge conflicts here?

[smuir]

Sure, will take a look

[smuir]

I'm not sure what the best way to resolve this is given the whole weirdness around repositories being changed to private and inaccessible, but I think (given the relatively small and clean nature of this change) that this might be most easily done by abandoning this PR and creating a fresh one. If you disagree speak up...

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}