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

Create Pipeline Rules syntax highlighting mode for Ace #5957

Merged
merged 20 commits into from
Jun 21, 2019

Conversation

kyleknighted
Copy link
Contributor

@kyleknighted kyleknighted commented May 21, 2019

Description

Created custom Mode for Ace Editor to introduce syntax highlighting for Pipeline Rules

Motivation and Context

  • User request
  • Improve code readability

How Has This Been Tested?

Screenshots (if appropriate):

Before:
image

After:
image

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@kyleknighted
Copy link
Contributor Author

Errors currently in master

client.js:772 POST http://localhost:9000/api/system/pipelines/rule/parse 400 (Bad Request)

FetchProvider.js:19 There was an error fetching a resource: cannot POST http://localhost:9000/api/system/pipelines/rule/parse (400). Additional information: Not available

After adding in the syntax highlighting, an additional error is added to the mix, even though the mode actually loads fine and renders appropriately

index.js:3802 GET http://127.0.0.1:8080/system/pipelines/rules/mode-pipeline.js net::ERR_ABORTED 404 (Not Found)

I could use an additional set of eyes to help me figure out what's causing this 404 even though it actually works.

@kyleknighted kyleknighted force-pushed the ace_editor_pipeline_rules_mode branch 2 times, most recently from a83c08f to fd4ead0 Compare June 14, 2019 15:14
Copy link
Member

@dennisoelkers dennisoelkers left a comment

Choose a reason for hiding this comment

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

I have played with the PR today and it is a good improvement!

Some suggestions:

  • The mode should be named pipeline/pipeline_rule, not graylog to make it more specific.

  • The mode file can be reduced in size dramatically. While playing with it, I removed the JavaScript mode and let the graylog mode inherit from TextMode. Also, the folding is not required.

  • The mode contains a number of keywords (match|pipeline|all|either) which are not used in the rule language. I think you got them from the internal representation of pipelines, which is different from pipeline rules. While we might support editing them at some point, it would be a different mode than the pipeline rules.

  • rule|when|then|end should be keywords, not constants. This way the highlighting lets you differentiate visually between the outer framing of the rule and the used functions:
    Screen Shot 2019-06-19 at 14 02 53

  • Instead of maintaining a huge list of functions, we should try to fetch them again from the backend. (Maybe by using an instance of the mode again, which is passed to react-ace, so it is not evaluated in its scope, but ours)

  • If that is not possible, we should replace the static list with a regex. IMO it is confusing if a function that is in the list has a different color than a function that is supplied by a plugin (but is still correct). Validating that a function is defined is done by the editor anyway.

@kyleknighted kyleknighted force-pushed the ace_editor_pipeline_rules_mode branch 2 times, most recently from babb9b8 to 5e9b8f8 Compare June 19, 2019 17:43
@kyleknighted kyleknighted force-pushed the ace_editor_pipeline_rules_mode branch from 5e9b8f8 to 4f67b2c Compare June 20, 2019 12:14
@edmundoa edmundoa merged commit d5539f5 into master Jun 21, 2019
@edmundoa edmundoa deleted the ace_editor_pipeline_rules_mode branch June 21, 2019 09:39
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.

4 participants