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

refactor(errors): one file per error #10355

Merged
merged 3 commits into from Jan 18, 2019
Merged

refactor(errors): one file per error #10355

merged 3 commits into from Jan 18, 2019

Conversation

@papb
Copy link
Member

@papb papb commented Jan 16, 2019

Pull Request check-list

Please make sure to review and check all of these items:

  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Have you added new tests to prevent regressions?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

All errors were defined in a single huge file. This PR refactors it so that every error has its file, and uses a folder structure for the nested error types.

I encountered this when I was about to contribute with a new error, but I felt like refactoring this first.

@SimonSchick
Copy link
Member

@SimonSchick SimonSchick commented Jan 16, 2019

I don't really see the advantage of this, the error logic is far from complicated and all this does is increase load time.

@codecov
Copy link

@codecov codecov bot commented Jan 16, 2019

Codecov Report

Merging #10355 into master will decrease coverage by 6.56%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10355      +/-   ##
==========================================
- Coverage   96.28%   89.71%   -6.57%     
==========================================
  Files          68       90      +22     
  Lines        9775     9726      -49     
==========================================
- Hits         9412     8726     -686     
- Misses        363     1000     +637
Impacted Files Coverage Δ
lib/errors/bulk-record-error.js 100% <100%> (ø)
lib/errors/connection-error.js 100% <100%> (ø)
lib/errors/database-error.js 100% <100%> (ø)
lib/errors/connection/access-denied-error.js 100% <100%> (ø)
lib/errors/database/timeout-error.js 100% <100%> (ø)
lib/errors/instance-error.js 100% <100%> (ø)
lib/errors/empty-result-error.js 100% <100%> (ø)
lib/errors/connection/host-not-reachable-error.js 100% <100%> (ø)
lib/errors/association-error.js 100% <100%> (ø)
...ib/errors/database/foreign-key-constraint-error.js 100% <100%> (ø)
... and 51 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 0f0255e...0c301e8. Read the comment docs.

Copy link
Contributor

@sushantdhiman sushantdhiman left a comment

Yeah I feel this file is too big and can be refactored without much impact on library. One change I would recommend to get rid of @extends directive from all errors for esdocs so they look like this

image

@papb
Copy link
Member Author

@papb papb commented Jan 16, 2019

@SimonSchick - I just tested on my machine here, using time-require:

  • It took 345.7 ms on average (out of 10 samples) without this PR
  • It took 354.5 ms on average (out of 10 samples) with this PR

I didn't compute the standard deviation for each case explicitly but the values ranged from ~330 to ~370 on both cases. To be honest I was surprised, I thought the load time would increase a lot more! Regardless, I believe this is a very small price to pay (the difference is eaten by the standard deviation!) for better readability and maintainability.

@sushantdhiman Nice catch! You're right. By removing it, esdoc parses correctly the inheritance chain by default! Nice! I just did it.

@SimonSchick
Copy link
Member

@SimonSchick SimonSchick commented Jan 16, 2019

@papb that is weird, when I did the mass cleanups before it increased load times by almost 10% which is why I omitted it from my PR, I guess this depends on the system it's running.

My previous test was using console.time on node v10 btw.

You should add trailing newlines btw.

Also @sushantdhiman what's the deal with the manual captureStackTrace, is it just to omit a stack frame?

@papb
Copy link
Member Author

@papb papb commented Jan 16, 2019

@SimonSchick - I see... I am on windows on Node 6 (old, I know, I'll do something about it later).

I've just added the trailing newlines as requested 😁

@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Jan 18, 2019

what's the deal with the manual captureStackTrace, is it just to omit a stack frame?

@SimonSchick Looks like that

@sushantdhiman sushantdhiman merged commit b875b7a into sequelize:master Jan 18, 2019
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@SimonSchick
Copy link
Member

@SimonSchick SimonSchick commented Jan 18, 2019

@sushantdhiman as a future note, this can be removed once support for v6 is dropped, iirc it was a workaround for prototype shenanigans pre v4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants