Skip to content

Remove Watson references #14

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

Merged
merged 12 commits into from
Mar 13, 2019
Merged

Remove Watson references #14

merged 12 commits into from
Mar 13, 2019

Conversation

lpatino10
Copy link
Contributor

The motivation for this PR was that I realized we really didn't need to have the Watson base test classes in the core, which could remove the testing dependency introduced in this earlier PR.

Along with that removal, it made sense to me to do a larger-scale refactoring to remove Watson-specific logic/references in anticipation of other teams taking advantage of this core library.

@lpatino10
Copy link
Contributor Author

@padamstx three things:

  1. Let me know if you have a preferred name for the base service class (was WatsonService, now BaseService). I'm not married to the name but it felt descriptive enough at least.
  2. There might be some more specific pieces of logic that could maybe be moved to the Watson SDK that I left in for now because I didn't want to worry about trying to figure out how to put those together just yet. One that comes to mind is the credential file logic in CredentialUtils, which is only supported by Watson services at the moment. We can think more about how to separate those things in the future.
  3. This is technically a breaking change, but I don't know how strict you want to be with the semantic naming yet since the Watson SDK is the only user so far (as far as I know).

@lpatino10 lpatino10 requested a review from padamstx March 12, 2019 18:58
@padamstx
Copy link
Member

Oh man, I wished we had talked about this before these name changes. The cloud object storage team has a current snapshot of the classes contained in the core in anticipation of moving to the new java core package. I'll need to check with them about any potential impact if we rename a bunch of classes.

@lpatino10
Copy link
Contributor Author

Ahh okay. Well we don't need to change things now if it's going to be a hassle. Although at some point I imagine it would make sense to make things more generic and it might be easier to make that transition now.

@lpatino10
Copy link
Contributor Author

@padamstx I just looked through the changes to verify that it won't cause breaking issues with users of SDKs relying on this core.

The only thing that will change at that level is the removal of authentication using the old X-Watson-Authorization-Token header. This was deprecated anyway though, so I don't think it should be an issue.

Other than that, it's just renaming, which should be resolved with a PR to the generator to capture the new base service class name and a re-gen.

Copy link
Member

@padamstx padamstx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I never noticed that JNDI was mis-spelled JDNI. It's amazing what the human mind can filter out :)

@lpatino10 lpatino10 merged commit 3f67576 into master Mar 13, 2019
@lpatino10 lpatino10 deleted the remove-watson-references branch March 13, 2019 15:25
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.

3 participants