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

Handle newlines in error messages, fixes #1495 #1984

Merged
merged 1 commit into from Oct 29, 2017

Conversation

Projects
None yet
2 participants
@RodericDay
Copy link
Contributor

RodericDay commented Oct 11, 2017

Description

Moves an inline match call into its own function (o.fn level), adds a failing test to that function, then fixes its behavior.

Motivation and Context

Fixes #1495

How Has This Been Tested?

Created a new function, ran node ospec/bin/ospec on it

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • [~] My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have updated docs/change-log.md

@RodericDay RodericDay requested a review from lhorie as a code owner Oct 11, 2017

@RodericDay RodericDay force-pushed the RodericDay:newlines-in-error-messages branch from f22c666 to dbb003c Oct 11, 2017

@RodericDay RodericDay requested a review from tivac as a code owner Oct 11, 2017

@RodericDay RodericDay force-pushed the RodericDay:newlines-in-error-messages branch from dbb003c to 8fe7088 Oct 11, 2017

@isiahmeadows
Copy link
Collaborator

isiahmeadows left a comment

LGTM

@@ -49,6 +49,9 @@ module.exports = new function init(name) {
spy.callCount = 0
return spy
}
o.cleanStackTrace = function(stack) {
return stack.match(/^(?:(?!Error|[\/\\]ospec[\/\\]ospec\.js).)*$/gm).pop()

This comment has been minimized.

@isiahmeadows

isiahmeadows Oct 11, 2017

Collaborator

Not a critical nit, but this isn't safe for if ospec is bundled. (This isn't a problem for Mithril, but some of its users might get tripped up on it.)

This comment has been minimized.

@RodericDay

RodericDay Oct 11, 2017

Contributor

I... don't really get it. I just placed that funtion there cause I wanted to have it accessible in the test, but still encapsulated within the o object. I'd happily move it.

This comment has been minimized.

@isiahmeadows

isiahmeadows Oct 11, 2017

Collaborator

To clarify, the code snippet I'm referring to was there before your patch.

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Oct 29, 2017

Merging, since this seems to have fallen through the cracks...

@isiahmeadows isiahmeadows reopened this Oct 29, 2017

@isiahmeadows isiahmeadows merged commit 3c608f2 into MithrilJS:next Oct 29, 2017

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment