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

#3216 JQ fromdate error gets cut off in page editor #3336

Merged
merged 8 commits into from
May 6, 2022

Conversation

BALEHOK
Copy link
Contributor

@BALEHOK BALEHOK commented May 5, 2022

Fixes #3216

@BALEHOK BALEHOK requested review from fregante, twschiller and BLoe May 5, 2022 14:04
src/errors.ts Outdated
@@ -250,6 +250,16 @@ export function isContextError(error: unknown): error is ContextError {
);
}

// The serializeError preserves custom properties that effectively means the "cause" is skipped by the serializer
export function serializePixiebrixError(error: Error): ErrorObject {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we keep this function it needs to be renamed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a link to the issue here? sindresorhus/serialize-error#74

@codecov-commenter
Copy link

codecov-commenter commented May 5, 2022

Codecov Report

Merging #3336 (86a7f92) into main (52ec942) will increase coverage by 0.11%.
The diff coverage is 93.75%.

@@            Coverage Diff             @@
##             main    #3336      +/-   ##
==========================================
+ Coverage   36.77%   36.89%   +0.11%     
==========================================
  Files         751      751              
  Lines       21509    21526      +17     
  Branches     4590     4595       +5     
==========================================
+ Hits         7911     7941      +30     
+ Misses      12682    12669      -13     
  Partials      916      916              
Impacted Files Coverage Δ
src/blocks/transformers/jq.ts 86.20% <88.88%> (+86.20%) ⬆️
src/background/logging.ts 21.64% <100.00%> (ø)
src/errors.ts 81.95% <100.00%> (+0.47%) ⬆️

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 52ec942...86a7f92. Read the comment docs.

message =
"Unexpected end of jq filter, are you missing a parentheses, brace, and/or quote mark?";
} else {
const jqMessageFromStack = jqStacktraceRegexp.exec(error.stack)?.groups
Copy link
Contributor

@fregante fregante May 5, 2022

Choose a reason for hiding this comment

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

You can write it this way to avoid the nested condition. Prettier will likely improve the wrapping further

Suggested change
const jqMessageFromStack = jqStacktraceRegexp.exec(error.stack)?.groups
message = jqStacktraceRegexp.exec(error.stack)?.groups?.message.trim()
?? "Invalid jq filter, see error log for details"

@@ -333,7 +334,7 @@ export async function recordError(
data,

// Ensure it's serialized
error: serializeError(maybeSerializedError),
error: serializePixiebrixError(maybeSerializedError as Error),
Copy link
Contributor

Choose a reason for hiding this comment

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

serializeError must accept unknown; as Error is unsafely asserting that unknown is an Error without actually checking for it.

src/errors.test.ts Show resolved Hide resolved
@@ -291,3 +293,22 @@ describe("selectError", () => {
);
});
});

describe("serializatin", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
describe("serializatin", () => {
describe("serialization", () => {

[]
);
const contextError = new ContextError("text context error", {
cause: inputValidationError,
Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed this was a bug:

This is a temporary issue so I wouldn't spend too much time on it 👍

@BALEHOK BALEHOK requested a review from fregante May 6, 2022 19:50
@twschiller twschiller added this to the 1.6.2 milestone May 6, 2022
Copy link
Contributor

@twschiller twschiller left a comment

Choose a reason for hiding this comment

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

I added some tests 📈 and added some comments to clarify the context around that helper function a bit more. I also made it so the other errors thrown are BusinessErrors (indicating the brick is misconfigured)

@twschiller twschiller enabled auto-merge (squash) May 6, 2022 21:20
@twschiller twschiller merged commit 74b411e into main May 6, 2022
@twschiller twschiller deleted the bugfix/3216-jq-fromdate branch May 6, 2022 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

JQ fromdate error gets cut off in page editor
4 participants