Fix inconsistencies in the comments and some spaces - #42
Conversation
telesvar
left a comment
telesvar
left a comment
There was a problem hiding this comment.
Great changes, but I disagree with some. Please, check the comments ✌
|
|
||
| // If the response returned an error, try to get a Firebase error code/message. | ||
| // Sometimes the error codes are joined with an explanation, we don't need that(its a bug). | ||
| // Sometimes the error codes are joined with an explanation, we don't need that (its a bug). |
There was a problem hiding this comment.
I think, "it's a bug" should be fixed too
| /** | ||
| * Makes sure the user is signed-in and has up-to-date credentials. | ||
| * @throws Will throw if the user is not signed-in. | ||
| * Makes sure the user is logged in and has up-to-date credentials. |
There was a problem hiding this comment.
Maybe logged in should be signed in everywhere? I long for "sign in" as this verb is used in the API.
| * @param {boolean} [updateStorage = true] Check whether to update localStorage or not. | ||
| * Updates the user data in the localStorage. | ||
| * @param {Object} userData The new user data. | ||
| * @param {boolean} [updateStorage = true] Whether to update local storage or not. |
There was a problem hiding this comment.
Check is essential here, otherwise the sentence is incomplete.
Also, I did not think about storage method can be replaced and may not be localStorage exclusively.
Maybe, "localStorage" should be replaced by "browser storage" in the comments?
There was a problem hiding this comment.
This lib is used by some outside of the browser, that's why I try to avoid using any other term.
And localStorage is the name of the storage we are currently using by default, so I think its OK to use.
As for "check". I think the change is OK.
| * Sets up a function that will be called whenever the user state is changed. | ||
| * @param {function} callback Function to call when the event is triggered. | ||
| * @returns {function} The function that unsubscribes your callback after being called. | ||
| * Set up a function that will be called whenever the user state is changed. |
There was a problem hiding this comment.
Why removing s at the end of the "Set"?
|
@DaniyelMe please look into some of the suggested changes made by @aidarkhanov. |
|
@aidarkhanov can you please take a look to the changes/fixes I made. |
telesvar
left a comment
telesvar
left a comment
There was a problem hiding this comment.
Great!
But I realized, that "signed-in", "signed-out", "sign-in", "sign-out" should all be "signed in", "signed out", "sign in" and "sign out" as these are verbs. Therefore, dash there is incorrect.
Can you also fix all appearances of these verbs in the comments?
| /** | ||
| * Makes sure the user is logged in and has up-to-date credentials. | ||
| * @throws Will throw if the user is not signed in. | ||
| * Makes sure the user is signed-in and has up-to-date credentials. |
There was a problem hiding this comment.
Great!
But I realized, that "signed-in", "signed-out", "sign-in", "sign-out" should all be "signed in", "signed out", "sign in" and "sign out" as these are verbs. Therefore, dash there is incorrect.
Can you also fix all appearances of these verbs in the comments?
| */ | ||
| async enforceAuth() { | ||
| if (!this.user) throw Error('The user must be logged-in to use this method.'); | ||
| if (!this.user) throw Error('The user must be signed-in to use this method.'); |
|
@DaniyelMe Take your time, but I think you should work on a different branch, and then make a pull request to master. All of your changes show as conflicts, and the name of both branches is the same so it makes it a little difficult to resolve. |
|
@samuelgozi Sorry for the mess, I've fixed all the merge conflicts and @aidarkhanov great suggestions. Please let me know if I missed something. |
3 participants
These are inconsistencies in the comments, and missing spaces.