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

defaultOptions for js #1568

Merged
merged 7 commits into from
Nov 1, 2018
Merged

Conversation

zilioti
Copy link
Contributor

@zilioti zilioti commented Oct 6, 2018

Hi @bitwiseman,
since I worked in the tests for options core in #1446, I decided to take a chance at doing #1364.

I opened this PR with the basic idea to check if the below implementation is desired by @tkrotoff:

import { html_beautify } from 'js-beautify';

const excludeButton = html_beautify.defaultOptions.unformatted.filter(tag => tag !== 'button');
const niceHtml = html_beautify(uglyHtml, { unformatted: excludeButton });

Also, could you guide me where should I test? I made just a basic one for now.

@bitwiseman
Copy link
Member

@zilioti
The defaultOptions should probably be functions that return new instances of each Options when called. Otherwise someone could change the defaultOptions and side-effect unexpectedly.

Other than that this seems great.

@bitwiseman
Copy link
Member

@zilioti
Also this will need be exposed via the legacy packages.

@zilioti
Copy link
Contributor Author

zilioti commented Oct 20, 2018

Hi @bitwiseman,

I changed the defaultOptions to be a function.
Regarding the legacy packages, I think that was already done. I added a test to check it 😄!

Copy link
Member

@bitwiseman bitwiseman left a comment

Choose a reason for hiding this comment

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

This addresses the issue as filed.
I'll open a new issue to add a defaultOptions to the main beautifier object in js/src/index.js that gives a cleaned up default options object.

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.

2 participants