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.

Sign up for GitHub

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

Jump to bottom

Fix setOptions to the ConsoleReporter if it exists on the options param #98

Merged
merged 1 commit into from
Dec 2, 2016
Merged

Fix setOptions to the ConsoleReporter if it exists on the options param #98

merged 1 commit into from
Dec 2, 2016

Conversation

cnishina
Copy link
Contributor

@cnishina cnishina commented Oct 28, 2016 •
edited
Loading

  • move default values for setOptions to the var when defined
  • setOptions to the ConsoleReporter if it exists on the options param
  • test setOptions should not override existing options if set multiple times

closes #95

This was referenced Oct 28, 2016
Closed
Closed
@cnishina
Copy link
Contributor Author

cnishina commented Nov 7, 2016

@slackersoft could you review this for me? Thanks!

@slackersoft
Copy link
Member

slackersoft commented Nov 8, 2016 •
edited
Loading

I don't think this is quite what we want to solve this issue. The thing we would want, is to only call configureDefaultReporter if it has not already been called. It looks like the reportersCount variable should also be deleted at some point since it isn't used.

Additionally, you have not provided any tests for your functionality. Please review and update this pull request or create a new one to address these issues. Thanks for using Jasmine!

@cnishina
Copy link
Contributor Author

cnishina commented Nov 8, 2016

Good point. Looking at this more, I'll have a fix and some tests. I'll ping you again when I have everything completed.

@cnishina cnishina changed the title fix - prevent execute method from overriding configureDefaultReporter Fix setOptions to the ConsoleReporter if it exists on the options param Nov 8, 2016
@cnishina cnishina force-pushed the fix_defaultreporter branch 2 times, most recently from e5cfb1e to 2ae4cb6 Compare November 8, 2016 07:08
@cnishina
Copy link
Contributor Author

cnishina commented Nov 8, 2016

@slackersoft It looks like ConsoleReporter setOptions was overriding previous values such as print so I made the changes there and have included a test. I checked the test in comparison to the previous version with setOptions and it would report an error with print is not a function.

I reverted back removing Jasmine.prototype.provideFallbackReporter since it did not appear to be used. If this makes sense, I'll could put together a PR to remove that and reportersCount.

@cnishina
Copy link
Contributor Author

@slackersoft pinging again for review. Thanks!

slackersoft
slackersoft requested changes Dec 1, 2016
View reviewed changes
lib/reporters/console_reporter.js
@@ -10,7 +10,7 @@ function ConsoleReporter() {
showColors = false,
timer = noopTimer,
jasmineCorePath = null,
printDeprecation = function() {},
printDeprecation = require('../printDeprecation'),
Copy link
Member

Choose a reason for hiding this comment

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

We're actually doing this in the previous manner so that no deprecation errors are printed out during specs. This also allows the specs not not have to pass in a custom no-op printDeprecation if that particular spec isn't concerned with the deprecated functionality.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted printDeprecation to be a no-op.

@cnishina
Fix setOptions to the ConsoleReporter if it exists on the options param
1d51773 
- setOptions to the ConsoleReporter if it exists on the options param
- test setOptions should not override existing options if set multiple times

closes jasmine#95
@slackersoft slackersoft merged commit 1d51773 into jasmine:master Dec 2, 2016
slackersoft pushed a commit that referenced this pull request Dec 2, 2016
Merge branch 'fix_defaultreporter' of https://github.com/cnishina/jas…
f5d1d09 
…mine-npm into cnishina-fix_defaultreporter

- Merges #98 from @cnishina
- See also #95
@cnishina
Copy link
Contributor Author

cnishina commented Dec 2, 2016

@slackersoft Thanks!

@cnishina cnishina mentioned this pull request Dec 9, 2016
Closed
@cnishina cnishina mentioned this pull request Jan 3, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slackersoft slackersoft slackersoft approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

configureDefaultReporter() options are lost
2 participants
@cnishina @slackersoft