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

Added Support for Bro language #925

Merged
merged 8 commits into from
Apr 8, 2016
Merged

Added Support for Bro language #925

merged 8 commits into from
Apr 8, 2016

Conversation

wayward710
Copy link
Contributor

Added support for Bro language.

@wayward710
Copy link
Contributor Author

Added support for Bro language. Please let me know if you need any additions or changes.

@wayward710 wayward710 changed the title Master Added Support for Bro language Apr 1, 2016
@wayward710
Copy link
Contributor Author

Please ignore the change I made to examples.js -- I did that for local testing purposes.

@zeitgeist87
Copy link
Collaborator

Hi @wayward710

Thanks for your contribution! Bro is a really interesting language.

Here are some things I've noticed about your PR:

  • There are some minor whitespace issues in your PR. You mixed tabs and spaces for indentation. You should only use tabs.

  • Please ignore the change I made to examples.js -- I did that for local testing purposes.

    Well you have to remove it from the PR eventually 😄

  • You overuse the array syntax for tokens a little bit. The performance is actually quite bad, so it is better to avoid it if possible. There are other issues too. For example the order of the array elements is important. The first elements are matched first and can prevent longer and better matches from other elements down the line.

  • You should use non-capturing groups whereever possible \b(?:keyword1|keyword2)\b

  • On the example page you use the module keyword in an example, but it is missing in the language definition.

  • I don't know anything about Bro, but in your example, the redef builtin seems to be used without the leading ampersand. Is the ampersand part of the builtin keyword or more like an operator? On this page it seems to be highlighted as an operator https://www.bro.org/sphinx/scripting/index.html


'boolean': /\b(T|F)\b/,

'keyword': [
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can use a single regex instead of the array here. You should also use a non-capturing group.

@zeitgeist87
Copy link
Collaborator

I am away for the weekend, so I cannot answer any questions until Monday...

@wayward710
Copy link
Contributor Author

Thanks for your feedback! I'm also traveling this weekend, so it may be a few days before I can work on the changes.

@wayward710
Copy link
Contributor Author

OK, I think I fixed the issues that you pointed out. Please let me know if anything else needs to be changed.

@zeitgeist87
Copy link
Collaborator

Hi @wayward710,

Thanks! It looks really nice now.

There are only a few small issues. Other than that I think it is ready to be merged.

  • You still have some whitespace issues. I know it is not all that important, but it just looks unprofessional:
    whitespace
    You should configure your editor to display whitespace characters and automatically remove trailing whitespace on save.

  • Your tokens seem to be in a random order, but the order is important, because the tokens are matched by Prism in that order. For example operator and punctuation should be last. Operators that are matched too early can prevent later matches. For example the minus sign in @load-sigs is matched as an operator:
    loadsigs

  • I don't know Bro, but I assume that this 'italic': /\b(TODO|FIXME|XXX)\b/, should be inside of a comment?

    'comment': {
    pattern: /(^|[^\\$])#.*/,
    lookbehind: true,
    inside: {
        'italic':  /\b(TODO|FIXME|XXX)\b/,
    }
    },
  • What minimizer do you use? If I use gulp to generate prism-bro.min.js I get a different result than yours. Maybe you used another version of gulp...

  • When you are finished you should also squash all your commits down into one or two nice commits, so that we don't have all the corrections in the log.

@wayward710
Copy link
Contributor Author

OK, I fixed the whitespaces, reordered the tokens, fixed the italics, and used Gulp for magnification instead of the other tool.

@zeitgeist87 zeitgeist87 merged commit 3b997fb into PrismJS:master Apr 8, 2016
@zeitgeist87
Copy link
Collaborator

Thanks for your contribution!

It would be really nice if you could create a few test cases for the Bro language. The tests are in the directory tests/languages. You can run the tests with the command npm test. The syntax should be straight forward.

@zeitgeist87
Copy link
Collaborator

I missed that this was a PR for the master branch. You should create your PR for the gh-pages branch. I moved your commits there and removed them from master.

@Golmote
Copy link
Contributor

Golmote commented Apr 8, 2016

Guidelines regarding how to write tests can be found here: http://prismjs.com/test-suite.html

@wayward710
Copy link
Contributor Author

OK, and use the gh-pages branch for the test suites?

@Golmote
Copy link
Contributor

Golmote commented Apr 8, 2016

Yes. All PRs should be done on gh-pages ;)

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