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

Whitespace normalizer plugin #847

Merged
merged 2 commits into from
Feb 6, 2016

Conversation

zeitgeist87
Copy link
Collaborator

This plugin normalizes whitespace in code blocks. It can perform
various operations. The user can configure them through the plugin
object located at Prism.plugins.WhitespaceNormalizer.

Prism.plugins.WhitespaceNormalizer.setOptions({
    removeTrailingWhitespace: true,
    removeExtraIndent: true,
    removeExtraIndent: true,
    leftTrim: true,
    rightTrim: true,
    tabsToSpaces: 8,
    //spacesToTabs: 4,
});

The plugin can be disabled for certain code blocks, by adding the
class "no-whitespace-normalization" to it.

@zeitgeist87
Copy link
Collaborator Author

My idea for this PR was to create a standardized way for plugins to get access to global configuration options. But I am really not sure if this is the right way to do it. We could also do it with classes or data-* attributes. This is just an experiment. I am open for suggestions and feedback 😄

@cmcculloh-kr
Copy link

I've tried this on at least 30 different HTML and JS Prism highlighted snippets and have not seen any issues. Thanks!

@zeitgeist87
Copy link
Collaborator Author

@CormacMcCarthy Thank you for testing it!

@zeitgeist87
Copy link
Collaborator Author

I am thinking about removing the Plugin base class. It is doesn't do much and the whitespace normalizer works perfectly fine without it.

@zeitgeist87 zeitgeist87 changed the title Object-oriented plugin framework and whitespace normalizer Whitespace normalizer plugin Dec 31, 2015
@LeaVerou
Copy link
Member

LeaVerou commented Jan 2, 2016

Thanks Andreas, we definitely need something like this!

A few remarks:

  • In general, a design goal of Prism is to be usable by people with no JS knowledge. This is why it highlights automatically, and this is why most plugins accept their options as data-xxx attributes and act on anything with a certain class or attribute without requiring anyone to manually create a new instance. Requiring JS for advanced usage is ok, but as long as there's no real reason for a plugin to require JS knowledge, I'd rather it had at least a basic HTML API. We often don't realize, but there are a LOT of people getting by with only HTML and CSS, and struggle with writing even a single line of JS (fun fact: my research at MIT is about that and making things easier for these people).
  • Why convert tabs to spaces? That seems a bit backwards when we have the CSS tab-size. I hate it when people do that just because they like spaces, then I copy their code and have a bunch of spaces in my tab-indented code. Interestingly, nobody seems to do the opposite (not that I'd advocate for it, I think it would be an equally bad idea).
  • What are the defaults of these options? Sensible defaults are of utmost importance for good usability.
  • The options are a bit long and they would be even more unwieldy as data- attributes. But I’m aware I tend to favor overly short names, so I'd merge it regardless :)
  • "Normalize Whitespace" would probably be a name more consistent with the rest of the plugins.

Thanks again! Don't be discouraged by the long list above, I really do appreciate the work you've put into this, and we badly need it :)

@zeitgeist87
Copy link
Collaborator Author

In general, a design goal of Prism is to be usable by people with no JS knowledge. This is why it highlights automatically, and this is why most plugins accept their options as data-xxx attributes and act on anything with a certain class or attribute without requiring anyone to manually create a new instance. Requiring JS for advanced usage is ok, but as long as there's no real reason for a plugin to require JS knowledge, I'd rather it had at least a basic HTML API. We often don't realize, but there are a LOT of people getting by with only HTML and CSS, and struggle with writing even a single line of JS (fun fact: my research at MIT is about that and making things easier for these people).

Yes that is exactly the kind of problem I was struggling with. Currently Prism has no standard way to allow a user to change global settings for plugins. My solution involves a bit of JS, but it is quite declarative and there are sensible defaults:

<script src="plugins/whitespace-normalizer/prism-whitespace-normalizer.js"></script>
<script type="text/javascript">
Prism.plugins.WhitespaceNormalizer.setOptions({
    rightTrim: false,
});
</script>

I agree a basic HTML API can't hurt. I'll start working on it.

Why convert tabs to spaces? That seems a bit backwards when we have the CSS tab-size.

I completely agree. Both of these options are disabled by default. I thought maybe there is a use for someone.

What are the defaults of these options? Sensible defaults are of utmost importance for good usability.

The defaults are set in the constructor:

Prism.plugins.WhitespaceNormalizer = new WhitespaceNormalizer({
    removeTrailingWhitespace: true,
    removeExtraIndent: true,
    leftTrim: true,
    rightTrim: true,
    //tabsToSpaces: 4,
    //spacesToTabs: 4,
});

The options are a bit long and they would be even more unwieldy as data- attributes. But I’m aware I tend to favor overly short names, so I'd merge it regardless :)

The names have to match the names of the methods of the WhitespaceNormalizer object. I used the Java naming conventions for methods. But I can use shorter names that are a better fit for data-* attributes.

"Normalize Whitespace" would probably be a name more consistent with the rest of the plugins.

I'll change it.

@LeaVerou
Copy link
Member

LeaVerou commented Jan 3, 2016

An HTML API doesn't have to be on every element that is highlighted. Prism solves that problem by inheriting the .language-xxx class from ancestors, so you could set it on any common ancestor. In practice, I find that much more pleasant to use than setting options via JS and it doesn't exclude those of us who are less familiar with JS. Of course, there should be a JS API as well.
Perhaps you could write a general framework so that people can have plugins with options that are specified via inheritable data- attributes :)

@LeaVerou
Copy link
Member

LeaVerou commented Jan 3, 2016

And yes, Java conventions result in huge names, to the point that Java developers are often made fun of for it :P Thankfully, native JS APIs are moving away from this (e.g. things like element.contains() vs element.compareDocumentPosition()).

@zeitgeist87
Copy link
Collaborator Author

Perhaps you could write a general framework so that people can have plugins with options that are specified via inheritable data- attributes :)

That's a great idea! I'll work on it. It shouldn't be too hard to implement. Why didn't I think of that 😄

And yes, Java conventions result in huge names, to the point that Java developers are often made fun of for it :P Thankfully, native JS APIs are moving away from this (e.g. things like element.contains() vs element.compareDocumentPosition()).

Yeah I know. I am usually the one who makes fun of Java developers for that. Nevertheless I like camel case for method names. The shorter the better of course.

@zeitgeist87 zeitgeist87 force-pushed the WhitespaceNormalizer branch from 6168f71 to c7e2160 Compare January 4, 2016 21:41
@zeitgeist87
Copy link
Collaborator Author

I have added another plugin to this PR. The plugin parses inheritable data-* attributes and classes and stores the result in the environment object ènv.settings. The Normalize Whitespace plugin uses the Parse Settings plugin to get its settings. The JS interface is still available.

@zeitgeist87 zeitgeist87 force-pushed the WhitespaceNormalizer branch from c7e2160 to fe2c1e4 Compare January 4, 2016 22:44
@LeaVerou
Copy link
Member

LeaVerou commented Jan 5, 2016

Sorry to spring more work on you, but can we split the two PRs?
Adding the general framework for inheritable data properties is a separate PR and the normalize whitespace plugin is a separate one. The former might even be added in Prism Core if we polish it enough (and hopefully, shorten it a bit).

@zeitgeist87
Copy link
Collaborator Author

I've just found an old PR #825. It's a Trim plugin, that does exactly the same thing as the Whitespace Normalizer. Argh, I should have checked the list of existing PRs before starting this plugin...

@LeaVerou
Copy link
Member

LeaVerou commented Jan 5, 2016

It's not your fault, there are a lot of PRs…

@zeitgeist87
Copy link
Collaborator Author

Sorry to spring more work on you, but can we split the two PRs?

I'd be happy to, but the Normalize-Whitespace plugin depends on the Parse-Settings plugin. If I separate them into two PRs the Normalize-Whitespace plugin wouldn't work and couldn't be tested properly.

PR #825 has some nice ideas. We should also consider it. The object-oriented approach of my Normalize-Whitespace plugin makes it a bit bulky compared to the simpler Trim plugin.

@zeitgeist87
Copy link
Collaborator Author

The former might even be added in Prism Core if we polish it enough (and hopefully, shorten it a bit).

I could do most of the stuff in a more functional programming style by using filter(), map(), forEach() etc. I didn't do that because I wanted to maximize performance. If code size is more important I can easily change it.

@LeaVerou
Copy link
Member

LeaVerou commented Jan 5, 2016

Yes, code size/readability/elegance are more important, unless of course there is a noticeable performance impact on average use cases. But avoiding map, forEach etc is premature optimization more often than not.

@zeitgeist87
Copy link
Collaborator Author

Yes, code size/readability/elegance are more important, unless of course there is a noticeable performance impact. But avoiding map, forEach etc is premature optimization more often than not.

Yes I think I am guilty of premature optimization 😄. I don't think that there is a noticeable performance impact. I will clean it up and submit another PR for the parse-settings plugin.

@LeaVerou
Copy link
Member

LeaVerou commented Jan 5, 2016

Yes I think I am guilty of premature optimization 😄

Heh, I know what you mean, I was too in the past. Then I realized that these differences shown in jsperf didn't matter for 99% of the projects, 99% of the time. However keeping the code small does, mainly so that humans can process and skim it quickly.

This plugin normalizes whitespace in code blocks. It can perform
various operations. The user can configure them through the plugin
object located at Prism.plugins.NormalizeWhitespace.

Prism.plugins.NormalizeWhitespace.setDefaults({
	'remove-trailing': true,
	'remove-indent': true,
	'left-trim': true,
	'right-trim': true,
	/*'indent': 2,
	'remove-initial-line-feed': false,
	'tabs-to-spaces': 4,
	'spaces-to-tabs': 4*/
});

The plugin can be disabled for certain code blocks, by adding the
class "no-whitespace-normalization" to it.

There is also a HTML-interface using the parse-settings plugin.
@zeitgeist87 zeitgeist87 force-pushed the WhitespaceNormalizer branch from fe2c1e4 to e86ec01 Compare January 7, 2016 09:48
@zeitgeist87
Copy link
Collaborator Author

I've removed the hard dependency on the parse-settings plugin, shortened the names of the options and fixed some minor bugs. The parse-settings plugin has its own PR #851.

This patch adds support for automatic line breaks after a
specific number of characters. The default is 80 characters, but
it can be set to any number.
@zeitgeist87
Copy link
Collaborator Author

I've added support for automatic line breaks to this PR.

LeaVerou added a commit that referenced this pull request Feb 6, 2016
@LeaVerou LeaVerou merged commit ef53322 into PrismJS:gh-pages Feb 6, 2016
@LeaVerou
Copy link
Member

LeaVerou commented Feb 6, 2016

Merged, thanks!

Sorry I haven't reviewed the parse settings plugin yet, I'll get to it!

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.

3 participants