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 #1241 #1424

Closed
wants to merge 1 commit into from
Closed

fix #1241 #1424

wants to merge 1 commit into from

Conversation

arBmind
Copy link

@arBmind arBmind commented Nov 9, 2014

perform stringify only if type is not string

perform stringify only if type is not string
@notslang
Copy link
Contributor

👍

@dasilvacontin
Copy link
Contributor

@boneskull's utils.stringify rework in #1357 should also fix this, but yeah, there's no logic running stringify on a string.

👍 from me, lacks tests, but we've never had them for this case. I prefer to merge this fix now rather than waiting for someone to code the tests.

I can commit to create the tests, if there isn't any other happy contributor/volunteer.

@boneskull
Copy link
Contributor

@dasilvacontin If you want to write some tests for the base reporter, that'd be awesome. Since we have none now, I'm not really itchy to merge this immediately.

@dasilvacontin
Copy link
Contributor

Sure, it will be fun. :) Assign #1429 to me and I'll try to get around it during next week.

@boneskull
Copy link
Contributor

@dasilvacontin apparently you cannot assign a ticket to anybody you like; only those with commit access

@dasilvacontin
Copy link
Contributor

Oh, I didn't know. No problemo.

@boneskull boneskull added the future needs to wait label Nov 25, 2014
@boneskull
Copy link
Contributor

This PR is waiting on tests.

@dasilvacontin
Copy link
Contributor

I should really get around writing those tests asap.

@boneskull boneskull added TO-MERGE and removed future needs to wait labels Dec 15, 2014
@boneskull boneskull self-assigned this Dec 15, 2014
@boneskull
Copy link
Contributor

I'll need to test this against my diff changes.

@dasilvacontin
Copy link
Contributor

Dang. I'm willing to pass the torch on this one, if anybody wants to code the tests for BaseReporter. I already made some, and I can publish/link the branch somewhere in order to checkout from there.

PS: 🙈

@boneskull
Copy link
Contributor

@dasilvacontin Don't worry about it, it'll be eviscerated w/ plugin api anyway

@boneskull boneskull closed this Mar 28, 2015
@boneskull
Copy link
Contributor

oh, wait, this is a PR.

@boneskull boneskull reopened this Mar 28, 2015
@dasilvacontin
Copy link
Contributor

@boneskull, yeah, what I'm not so sure is how this plays along with your diff changes, or a rework you made a month/s ago.

@jbnicolai jbnicolai force-pushed the master branch 3 times, most recently from 2f458ab to 2952eca Compare July 5, 2015 10:25
@boneskull
Copy link
Contributor

I'm going to have to close this due to conflicts. @arBmind thanks for the contribution

@boneskull boneskull closed this Jul 10, 2015
@boneskull boneskull removed their assignment Dec 12, 2017
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.

4 participants