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

Fix cloning errors #286

Merged
merged 3 commits into from
Jun 1, 2022
Merged

Fix cloning errors #286

merged 3 commits into from
Jun 1, 2022

Conversation

Allain55
Copy link
Contributor

Fixes #260

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@coveralls
Copy link

Pull Request Test Coverage Report for Build 1587052890

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 1586702094: 0.0%
Covered Lines: 318
Relevant Lines: 318

💛 - Coveralls

@@ -1,6 +1,6 @@
'use strict'

const clone = require('rfdc')({ circles: true })
Copy link
Member

Choose a reason for hiding this comment

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

Are there any other methods for resolving the issue at hand? This replacement module has not been updated in quite some time, has a long open dependabot PR, and planttheidea/fast-copy#39 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked every library with deep cloning and a minimal user base but I couldn't find any other library that can clone errors and would pass every test

Copy link
Member

Choose a reason for hiding this comment

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

Is there any other way, within this code base, outside of a cloning module?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

None that I can think of unfortunately

Copy link
Member

Choose a reason for hiding this comment

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

I'd like to hear from @davidmarkclements on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I think we can ship this

Copy link
Member

Choose a reason for hiding this comment

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

If I get a fix on rdfc, I'd be very happy to.

Copy link
Contributor

Choose a reason for hiding this comment

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

which fix are you talking about?

Copy link
Member

Choose a reason for hiding this comment

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

add support to rdfc to clone errors.

@@ -564,12 +564,12 @@ function deleteLogProperty (log, property) {
* Filter a log object by removing any ignored keys.
*
* @param {object} log The log object to be modified.
* @param {string} ignoreKeys An array of strings identifying the properties to be removed.
* @param {Set<string> | Array<string>} ignoreKeys An array of strings identifying the properties to be removed.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @param {Set<string> | Array<string>} ignoreKeys An array of strings identifying the properties to be removed.
* @param {string[]} ignoreKeys An array of strings identifying the properties to be removed.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina dismissed jsumners’s stale review June 1, 2022 07:45

rdfc does not clone errors

@mcollina mcollina merged commit 53b2ab4 into pinojs:master Jun 1, 2022
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.

customPrettifiers error doesn't work with ignore option
5 participants