Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set up an area for the enhanced 1.2.x interface. #27

Closed
wants to merge 1 commit into from
Closed

Set up an area for the enhanced 1.2.x interface. #27

wants to merge 1 commit into from

Conversation

comcast-jonm
Copy link
Member

  • This creates a new package specifically for the 1.2 interface.
    The idea is that we could collect interface enhancements in
    similar packages as we go on.
  • Deprecated the old Sirius (1.1.x) interface.

Discussion questions:

  • What else should go into this package? Ideally this should be everything that a client would need to interact with.
  • What do you think of creating a versioned package like this?
  • Is the package in the right place?
  • Does the package need a package.html (or whatever the ScalaDoc equivalent is)?
  • I suspect we will find we want classes like Sirius1Dot2Impl that are essentially interface adapters to a single SiriusImpl. One complicating factor is that SiriusImpl is a "public" class in 1.1.x because the SiriusFactory returns it. So probably the single, private core class needs a different name (SiriusFacade?).

References:

* This creates a new package specifically for the 1.2 interface.
  The idea is that we could collect interface enhancements in
  similar packages as we go on.
* Deprecated the old `Sirius` (1.1.x) interface.

Discussion questions:
1. What else should go into this package? Ideally this
   should be everything that a client would need to interact with.
2. What do you think of creating a versioned package like this?
3. Is the package in the right place?
4. Does the package need a `package.html` (or whatever the
   ScalaDoc equivalent is)?
5. I suspect we will find we want classes like `Sirius1Dot2Impl`
   that are essentially interface adapters to a single `SiriusImpl`.
   One complicating factor is that `SiriusImpl` is a "public" class
   in 1.1.x because the `SiriusFactory` returns it. So probably
   the single, private core class needs a different name
   (`SiriusFacade`?).
@smuir
Copy link
Contributor

smuir commented Feb 13, 2014

This looks like the right general approach, I'm not familiar enough with explicitly versioned APIs in Java/Scala to know if there's anything non-standard about this. One question about packages though: in one of the other open issues we seemed to settle on the top-level class (sirius) being the home for the public API classes, so maybe this should come under there, not under api.

@smuir
Copy link
Contributor

smuir commented Feb 13, 2014

I'm not sure what is the best way to share some code that builds on this to show at least the interface of the out-of-band data approach I suggested in #19. It doesn't even compile at this point, so I don't want to create a PR.

@comcast-jonm
Copy link
Member Author

I think it's ok to create a PR, just put (DO NOT MERGE) on the subject/summary line for it. It's a fine way to get feedback and it's ok for it not to build.

Alternatively, you can push it up to a branch on your GitHub fork and just point us to it.

@smuir
Copy link
Contributor

smuir commented Feb 14, 2014

Okay, I pushed it to branch sirius1dot2_tagged on my fork, and based it off of the @jonm-comcast sirius1dot2 changes. The commit history is a mess because I inadvertently incorporated some of my SiriusFactory changes, but the actual diffs vs the sirius1dot2 tree look correct.

@comcast-jonm
Copy link
Member Author

This PR is a prerequisite for #21, but we released 1.2.0 before this got in. I think I need to revise this and:

  • create the Sirius1Dot3 interface, as otherwise described
  • update trunk version number to 1.3.0-SNAPSHOT

Does that sound right?

@smuir
Copy link
Contributor

smuir commented Apr 26, 2014

Yes, I think that's the correct course of action

@comcast-jonm
Copy link
Member Author

I'm going to close this PR because it was generated from a lost repository that existed prior to making this repository public; also it needs to be revisited anyway since we've already released 1.2.x.

comcast-jonm added a commit to comcast-jonm/sirius that referenced this pull request Aug 7, 2014
This is a re-spin of Comcast#27 but also incorporates the intent of Comcast#68.
This should basically mean that a client of the Sirius library
ought only to directly use classes, objects, and traits from the
`com.comcast.xfinity.sirius.api` package, and that, going
forward, those are the files that must retain backwards
compatibility.

The main changes involved are:

* Additional extension methods for 1.3 are now collected in the
  Sirius1Dot3 trait that extends the existing Sirius trait.

* Existing SiriusFactory was hoisted up to api package and now
  returns a Sirius1Dot3 instead of a SiriusImpl. Clients ought
  to be depending on a trait rather than a concrete class here.

* There is still a SiriusFactory in the old location, but it
  just delegates to the one in the new location, then downcasts
  the return result to a SiriusImpl. This SiriusFactory is
  marked as deprecated.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants