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(CordovaError)!: remove unused features #117

Merged
merged 6 commits into from
Dec 12, 2019

Conversation

raphinesse
Copy link
Contributor

@raphinesse raphinesse commented Dec 11, 2019

Motivation and Context

CordovaError implements a lot of features that we do not use anywhere in our code base. I also find these features of little use. This PR removes them.

The result is an Error subclass that does nothing except for stripping the Error: prefix from the usual result of Error.prototype.toString. Thus we should probably discuss if we want to remove CordovaError completely in the future or extend it with useful functionality.

One thing that would be actually useful is accepting a second parameter reason of type Error like in Java. That way it's easy to add information when catching an error and re-throwing it. Actually, the one place in our code that constructs a CordovaError with more than one argument tries to do exactly that. Unfortunately that's not how the constructor works currently.

Of course this change is a breaking one and non-Apache cordova-common consumers would be affected by it as well. However, I don't expect there to be many that use CordovaError.

Description

Removed features:

  • support for an error context
  • support for Cordova-specific error codes
  • support for verbose toString

Testing

  • Still passes unit tests
  • Made sure we don't use the removed features anywhere

@raphinesse raphinesse added this to the 4.0.0 milestone Dec 11, 2019
@codecov-io
Copy link

codecov-io commented Dec 11, 2019

Codecov Report

Merging #117 into master will increase coverage by 0.65%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #117      +/-   ##
==========================================
+ Coverage   86.26%   86.91%   +0.65%     
==========================================
  Files          21       20       -1     
  Lines        1558     1529      -29     
==========================================
- Hits         1344     1329      -15     
+ Misses        214      200      -14
Impacted Files Coverage Δ
cordova-common.js 0% <ø> (ø) ⬆️
src/CordovaError/CordovaError.js 100% <100%> (+13.04%) ⬆️

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 2021a39...0cf9175. Read the comment docs.

Copy link
Contributor

@brodycj brodycj left a comment

Choose a reason for hiding this comment

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

👍

I hope we can look into adding the reason code soon, ideally in multiple places, as suggested in the description.

@raphinesse raphinesse merged commit 641be1e into apache:master Dec 12, 2019
@raphinesse raphinesse deleted the remove-unused-error-features branch December 12, 2019 21:43
raphinesse added a commit that referenced this pull request Dec 13, 2019
Moves the `CordovaError` module from `src/CordovaError/CordovaError.js` to `src/CordovaError.js`. The additional nesting makes no sense anymore since `src/CordovaError/CordovaExternalToolErrorContext.js` has been removed in #117.
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