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

Add debug function #1469

Closed
wants to merge 1 commit into from
Closed

Conversation

SergiCL
Copy link
Contributor

@SergiCL SergiCL commented Mar 14, 2020

This PR contains the changes discussed in #1461

What kind of change does this PR introduce? (check at least one)

  • Feature

Does this PR introduce a breaking change? (check one)

  • No

The PR fulfills these requirements:

@lmiller1990
Copy link
Member

lmiller1990 commented Mar 16, 2020

Great work. I personally do not want to make the API even bigger at this point - I also think debug is the wrong name for this function, since it does not debug anything, it just prints html. debug suggests it will drop you into a kind of console debugger (like pry from perl/python/rub or a JS debugger, like you get in IDEs such as WebStorm.

For this reason, I think this makes way more sense as an option to html. Eg: console.log(wrapper.html({ pretty: true }). We could also set this via a config variable to make this the default, is the users chooses to do so.

@afontcu @JessicaSachs @dobromir-hristov thoughts?

@dobromir-hristov
Copy link
Contributor

Yeah same.

@Stoom
Copy link
Contributor

Stoom commented Mar 27, 2020

It looks like the current wrapper.html() already pretty prints. From the look at the code, this just does a console.log (which doesn't seem to be desirable) and loops through a WrapperArray.

@SergiCL
Copy link
Contributor Author

SergiCL commented Mar 27, 2020

I'm going close this to avoid confusion and open a new one with the changes described in #1461 when done.

@SergiCL SergiCL closed this Mar 27, 2020
@SergiCL SergiCL deleted the pr/wrapper-debug-function branch March 27, 2020 13:05
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.

None yet

4 participants