Refactoring `User´ #268
Refactoring `User´ #268
Conversation
Deprecation of individual setters. Addition of `Client.getUser()` to make the ID accessible.
Those are some pedantic rule sets... I think single line IFs are A OKAY as are >100LL in 2018... 😞
Pull Request Test Coverage Report for Build 1190
💛 - Coveralls |
|
|
Yep, it's a bit of a flaky test on Travis. |
| import android.support.annotation.NonNull; | ||
| import android.support.annotation.Nullable; | ||
|
|
||
| import java.io.IOException; | ||
|
|
||
| /** | ||
| * Information about the current user of your application. | ||
| */ | ||
| class User implements JsonStream.Streamable { |
There was a problem hiding this comment.
This class would need to be public if exposed as part of the API
| private String name; | ||
| @Nullable private String id; | ||
| @Nullable private String email; | ||
| @Nullable private String name; |
There was a problem hiding this comment.
These fields can now be final
| User() { | ||
| } | ||
|
|
||
| User(String id, String email, String name) { |
There was a problem hiding this comment.
I think for now it's worth keeping this as a public convenience constructor that builds a Builder.
There was a problem hiding this comment.
I disagree. This was never public, so why would we want to introduce something new, "for now", that would be deprecated along with the other APIs?
Exposing only the builder to create User objects is what will make it easier to add additional user fields in the future, while keeping the API clean.
|
|
||
| User(@NonNull User user) { | ||
| this(user.id, user.email, user.name); | ||
| User(Builder builder) { |
There was a problem hiding this comment.
Making this private will generate a synthetic accessor call due to access from User.Builder.build() as there is no advantage to making private I'd prefer to avoid increasing the method count unnecessarily by one (once deprecated methods are removed, bugsnags method count be reduced even more which is always nice for a library).
| */ | ||
| public static Builder builder(User user) { | ||
| return new Builder(user); | ||
| } |
There was a problem hiding this comment.
I think we can get rid of the two methods above, and favour one way of creating a builder:
User user = new User.Builder().id("Foo").build();
The second method could be replaced by something like the following:
User.Builder builder = user.toBuilder();
There was a problem hiding this comment.
Good idea, this would also simplify internal use as user.toBuilder()... is nicer than User.builder(user)....
There was a problem hiding this comment.
I will also drop the User.builder() method too to save another method as the builder constructors are public anyways.
| public static class Builder { | ||
| @Nullable String id; | ||
| @Nullable String email; | ||
| @Nullable String name; |
There was a problem hiding this comment.
Fields can be private
There was a problem hiding this comment.
As there is no advantage to making them private, we should keep them package protected as this would save another 3 methods (synthetic accessors).
| public Builder(User user) { | ||
| id(user.getId()); | ||
| email(user.getEmail()); | ||
| name(user.getName()); |
There was a problem hiding this comment.
A test case for null users would be useful here
There was a problem hiding this comment.
I agree, also found a mistake in Error that could lead to NPE, fixing that now.
| .remove(USER_NAME_KEY) | ||
| .apply(); | ||
| notifyBugsnagObservers(NotifyType.USER); | ||
| public User getUser() { |
There was a problem hiding this comment.
Would be nice to add @see #setUser(User) to the JavaDoc
| */ | ||
| public void setUserId(String id) { | ||
| setUserId(id, true); | ||
| public void setUser(@Nullable User user) { |
There was a problem hiding this comment.
The JavaDocs for this method should include as much information as before, although we'll need to change how the params are handled:
Set details of the user currently using your application.
- * You can search for this information in your Bugsnag dashboard.
- * <p/>
- * For example:
- * <p/>
- * client.setUser("12345", "james@example.com", "James Smith");
- *
- * @param id a unique identifier of the current user (defaults to a unique id)
- * @param email the email address of the current user
- * @param name the name of the current user
There was a problem hiding this comment.
That information is available in the javadoc of User though? Which the dev will see when they construct a User object. By duplicating java doc here, everytime User is changed, the java doc has to be updated in 3 places instead of just in User where it belongs.
| * @param id a unique identifier of the current user (defaults to a unique id) | ||
| * @param email the email address of the current user | ||
| * @param name the name of the current user | ||
| * @param user The user information to set or NULL to reset to defaults. |
There was a problem hiding this comment.
Needs to include same information as before about id/email/name
|
|
||
| @Test | ||
| public void testRepo() { | ||
| SharedPreferences sharedPref = getSharedPrefs(InstrumentationRegistry.getContext()); |
There was a problem hiding this comment.
Would it be worth sharedPref.edit().clear().commit() beforehand here?
There was a problem hiding this comment.
Definitely, to guard against any future changes that could make this test flaky.
| } | ||
|
|
||
| @Test | ||
| public void testBuilder_values() { |
There was a problem hiding this comment.
Our codestyle uses camelcase, looks like I've got a bit of work to do configuring our checkstyle as it should have caught this D=
There was a problem hiding this comment.
Have you considered changing it? I think
testMethodA_defaults and testMethodA_nullArgument
is more readable than
testMethodADefaults and testMethodANullArguments
because it's immediately clear that it's two tests for the same method, just differents aspects.
I'll change it though, I don't have strong feelings about this.
The builders argument can now also be null.
Fixed unit tests method name style and added additional tests. Check that preferences are empty before testing.
|
What's the status on this? |
|
@kattrali is planning to review this with a second pair of eyes, but is currently at a conference for the rest of the week. |
|
Would like to see this merged as well ➕ |
|
Is there no interest in merging this? |
|
@fractalwrench @kattrali What's the issue here? |
|
@fractalwrench I'm annoyed that you encouraged a PR for this and now it just rots away 👎. Why did I waste my time on this? |
|
@d4rken I'm really sorry about how this was handled - it doesn't live up to our standards and I can only apologise that it happened. Shortly after this PR was opened, our policy on reviewing external PRs changed so that no longer accept changesets with feature additions or refactors of existing functionality. Coupled with an automation change that syncs GitHub issues to an internal ticketing system, I'm sorry to say that this PR slipped through the cracks. As a consequence of this, we will:
Again, I'd like to say how sorry we are - I've been in this position and know there's nothing more frustrating than spend time opening a PR and having it hang around for months. If there are any additional actions on our part that you think might be helpful in making this right, please let us know. |
|
😦
What caused this change?
👍
Don't worry, I'm still a happy customer. |
There's a few causes I can chime in on:
All that being said, we absolutely welcome feedback about using Bugsnag via either GitHub issues or support@bugsnag.com. Thanks again. |
Specify millisecond timeout correctly for entire request
test: support different .ipa outputs in MR build
User,Useris now immutable.Userand movedUserrelated implementation details fromClientto `User´Client.setUser(user)Userfields. In the next semvar major release these could be removed.Userinformation? Just updateUser.Useris now exposed viaClient.getUser()which solves my issue in Access user-id? #265@fractalwrench