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

@accounts/error : new AccountsError package with message formatting #171

Merged
merged 9 commits into from
Mar 14, 2018

Conversation

Aetherall
Copy link
Member

This is a first implementation of the AccountsError package.

new AccountsError('PasswordService','authenticate','no user match tokens')
// message => '[ Accounts - PasswordService ] authenticate : no user match tokens'

new AccountsError('no user match tokens')
// message => 'no user match tokens'

As the error code and login info is not used yet, I removed the code concerning them so we can implement it accordingly with the current state

@Aetherall Aetherall changed the title @accounts/error New AccountsError package with message formatting @accounts/error : new AccountsError package with message formatting Mar 12, 2018
Copy link
Member

@davidyaha davidyaha left a comment

Choose a reason for hiding this comment

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

We used to have optional login info on the Accounts Error to allow for audit logs in the future..
We never got to this future so I tend to approve removing this parts and add it later if needed..

},
"repository": {
"type": "git",
"url": "https://github.com/js-accounts/accounts/tree/master/packages/server"
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

my bad, will be fixed in the next minutes

@codecov-io
Copy link

codecov-io commented Mar 13, 2018

Codecov Report

Merging #171 into master will decrease coverage by 1.79%.
The diff coverage is 92.3%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #171     +/-   ##
=========================================
- Coverage   92.16%   90.37%   -1.8%     
=========================================
  Files          42       28     -14     
  Lines        1034      831    -203     
  Branches      128      112     -16     
=========================================
- Hits          953      751    -202     
  Misses         70       70             
+ Partials       11       10      -1
Impacted Files Coverage Δ
packages/error/src/index.ts 100% <100%> (ø)
packages/error/src/accounts-error.ts 91.66% <91.66%> (ø)
packages/rest-express/src/endpoints/get-user.ts
...es/rest-express/src/endpoints/password/register.ts
packages/rest-express/src/utils/get-user-agent.ts
packages/rest-express/src/express-middleware.ts
...t-express/src/endpoints/oauth/provider-callback.ts
packages/rest-express/src/endpoints/impersonate.ts
packages/rest-client/src/rest-client.ts
...rest-express/src/endpoints/service-authenticate.ts
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d4ac424...fcdc9f2. Read the comment docs.

@pradel
Copy link
Member

pradel commented Mar 13, 2018

@Aetherall is there a way to get 100% test coverage for the error package in oder to not loose 1.78% of code coverage for the global repository?

@Aetherall
Copy link
Member Author

Aetherall commented Mar 13, 2018

Actually the coverage missing comes from

if (typeof Error.captureStackTrace === 'function') {
  Error.captureStackTrace(this, this.constructor);
} else { 
  this.stack = (new Error(message)).stack; 
}

It would be a string comparison of something that is not predictable
Maybe there is an other way to do it I am not aware of ?

EDIT : Or I can do a !null comparison

EDIT 2 : I am having bad time trying to cover Error.captureStackTrace(this, this.constructor); If anyone have an idea ...

@Aetherall Aetherall mentioned this pull request Mar 13, 2018
12 tasks
@Aetherall
Copy link
Member Author

Can I merge This ?

@pradel
Copy link
Member

pradel commented Mar 14, 2018

You will need to prettier before I think and let's wait for @davidyaha review

@davidyaha
Copy link
Member

just figure out the tests but that is good to go IMO

@Aetherall Aetherall merged commit 4635cbe into master Mar 14, 2018
@Aetherall Aetherall deleted the package/error branch March 14, 2018 12:17
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.

4 participants