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

Substantial improvements to language-sas syntax highlighting #1981

Merged
merged 33 commits into from
Aug 8, 2019

Conversation

cedporter
Copy link
Contributor

  • Added numerous language elements
  • Created more granular keyword objects, for finer grained styling if desired
  • Expanded keyword regex list
  • Special identification of PROC SQL and the SQL that follows
  • Test coverage for all added language elements

* Added numerous language elements
* Created more granular keyword objects, for finer grained styling if desired
* Expanded keyword regex list
* Special identification of PROC SQL and the SQL that follows
* Test coverage for all added language elements
Copy link
Member

@RunDevelopment RunDevelopment left a comment

Choose a reason for hiding this comment

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

Thank you for providing this improvement!

But before we can merge this, there are some things we'll have to do. Besides the comments I left, it's mainly cleanup.
Regexes are inherently hard to read because of the compact syntax. Because of this, we have to extra careful to keep our regexes as simple and understandable as possible. Here are a few guidelines to make the regexes more readable:

  1. Only use groups if necessary.
    Unnecessary groups can make regexes appear a lot more complex than they actually are and hide potential mistakes. Avoid (\s)+ and (?:foo).
    That being said. Be careful not to remove a group too much!
  2. Remove unnecessary flags.
    Flags which aren't used in your pattern usually just confuse the reader when they try to understand a regex. Why is that flag there? Did I miss something?
  3. Don't unnecessarily escape characters.
    It's good to be on the safe side but don't escape everything. I.e. [\'\-|\"\/\\<>\*\w+=\(\)] is just a whole lot longer than [-'"|/\\<>*+=()\w] but both are equivalent.

In my comments, I also explained by I changed certain aspects of your regexes, so please take a look at those. Some of the changes I made (e.g. (^|\n)) can also be applied to other regexes.

I know that this sounds like a lot of work but it's worth it. It will make your regexes more readable and easier to understand.

If you have any question or need help, feel free to ask!

components/prism-sas.js Outdated Show resolved Hide resolved
components/prism-sas.js Outdated Show resolved Hide resolved
components/prism-sas.js Outdated Show resolved Hide resolved
components/prism-sas.js Outdated Show resolved Hide resolved
components/prism-sas.js Outdated Show resolved Hide resolved
components/prism-sas.js Outdated Show resolved Hide resolved
components/prism-sas.js Outdated Show resolved Hide resolved
components/prism-sas.js Outdated Show resolved Hide resolved
components/prism-sas.js Outdated Show resolved Hide resolved
components/prism-sas.js Outdated Show resolved Hide resolved
@RunDevelopment
Copy link
Member

@cedporter Could you please resolve the conflicts with master?

This will allow our CI to check your code as well.

Also, there are still comments left to address.

@RunDevelopment
Copy link
Member

@cedporter I'm sorry but could you please only mark comments as resolved after you pushed the changes which resolve them? From my perspective, comments are marked as resolved without actually being resolved which is a little confusing.

For example, I don't know whether you accidentally resolved this one or whether the commit just isn't pushed yet.

Thank you.

gulpfile.js/index.js Outdated Show resolved Hide resolved
components.json Outdated Show resolved Hide resolved
* hopefully fixing package-lock.json
* incorporating latest round of feedback
cedporter and others added 3 commits August 8, 2019 13:55
Co-Authored-By: Michael Schmidt <mitchi5000.ms@googlemail.com>
Co-Authored-By: Michael Schmidt <mitchi5000.ms@googlemail.com>
Copy link
Member

@RunDevelopment RunDevelopment left a comment

Choose a reason for hiding this comment

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

Please revert the package-lock.json and then I'll merge it.

@RunDevelopment RunDevelopment merged commit 076f615 into PrismJS:master Aug 8, 2019
@RunDevelopment
Copy link
Member

Thank you for contributing @cedporter!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants