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

Add OpenEdge ABL lexer #1033

Closed
wants to merge 2 commits into from
Closed

Add OpenEdge ABL lexer #1033

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Nov 15, 2018

No description provided.

@viitaset
Copy link

Any chance on getting this merged?

@ghost
Copy link
Author

ghost commented Apr 26, 2019

@viitaset apparently it's only waiting to be merged by @dblessing, see #1063

pyrmont referenced this pull request in pyrmont/rouge May 12, 2019
The branch that was the source of the original pull request (jneen#1033)
has been deleted. In order to run the CI tests, the pull request has
been checked out locally and this merges it with the other changes made
in the moulin branch.
@pyrmont pyrmont added the needs-review The PR needs to be reviewed label May 28, 2019
@pyrmont
Copy link
Contributor

pyrmont commented May 28, 2019

@raphaelhittich @viitaset Sorry this has taken so long. We're going through the backlog now and getting things merged in. I'll review this as soon as I can!

Copy link
Contributor

@pyrmont pyrmont left a comment

Choose a reason for hiding this comment

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

@raphaelhittich Sorry again this has taken a long time to get back to you. I'm looking at the code sample and thinking that some of my comments might be the result of OpenEdge just being pretty different to most of the languages I'm looking at. Please feel free to push back if you think I've misunderstood something fundamentally.

lib/rouge/lexers/openedge.rb Show resolved Hide resolved
lib/rouge/lexers/openedge.rb Show resolved Hide resolved
lib/rouge/lexers/openedge.rb Show resolved Hide resolved
lib/rouge/lexers/openedge.rb Show resolved Hide resolved
lib/rouge/lexers/openedge.rb Show resolved Hide resolved
lib/rouge/lexers/openedge.rb Show resolved Hide resolved
lib/rouge/lexers/openedge.rb Show resolved Hide resolved
lib/rouge/lexers/openedge.rb Show resolved Hide resolved
@pyrmont pyrmont added author-action The PR has been reviewed but action by the author is needed and removed needs-review The PR needs to be reviewed labels Jun 16, 2019
@pyrmont
Copy link
Contributor

pyrmont commented Jun 17, 2019

@raphaelhittich I've just realised that the structure of your states is based on the C lexer. Without having thought much more about it, it seems to me this approach is wrong but it makes sense why you wrote yours the way that you did.

Let me have a bit more of a think. I'll post again here when I have a clearer idea about how best to proceed.

@pyrmont pyrmont added maintainer-action The PR has been reviewed but action by a maintainer is required and removed author-action The PR has been reviewed but action by the author is needed labels Jun 17, 2019
@pyrmont
Copy link
Contributor

pyrmont commented Jun 17, 2019

OK, had a bit more of a think about things.

Putting to the side for the moment the question of whether the structure of the C lexer should be emulated, is this even appropriate for OpenEdge ABL? Perhaps the code samples I've found online aren't representative but it doesn't look like you need things like macro support (which should be the purpose of the :preproc state).

I'm hopeful that the scope of the language is less encompassing than it seems and a simplified lexer can be used instead.

The language seems pretty simple; did you use this structure for a particular reason? Are elements like string prefixes necessary? Or have these just been copied across from another lexer? I ask because there's nothing in the visual sample that suggests these features are part of the language.

@pyrmont pyrmont added author-action The PR has been reviewed but action by the author is needed and removed maintainer-action The PR has been reviewed but action by a maintainer is required labels Jun 17, 2019
@ghost
Copy link
Author

ghost commented Jun 17, 2019

@pyrmont Sorry, should have mentioned that I used the C lexer as my template, it was the simplest to go from for me.

OpenEdge ABL does have some kind of preprocessor, see this. So that's what the :preproc is for, it is also included in the example code, the first line after the comment &ANALYZE-SUSPEND _VERSION-NUMBER UIB_v8r12

I think most of your comments are just from the fact I used the C lexer as my template, except for the String#casecmp, you are correct, might be better to just use String#upcase in the assignment.

@pyrmont
Copy link
Contributor

pyrmont commented Jun 18, 2019

@raphaelhittich Did you delete your branch? It says 'Unknown repository' as the source for this PR. I ask because I've written a simplified version of the lexer that I think would make a better starting point but I can't propose it as a PR to your code as it appears your fork has been deleted.

In terms of my proposed lexer, the syntax highlighting could be improved by separating the keywords you've defined into functions, attributes, etc. I'm also not sure whether all of the rules are correct or whether they've just been carried across from the C lexer. Does OpenEdge really have string prefixes for example? Oh, and it wasn't clear to me whether periods at the end of statements can have whitespace after them. Your example does but I could imagine that that was a typo.

Anyway, happy to help more if you have any questions.

@ghost
Copy link
Author

ghost commented Jun 19, 2019

@pyrmont Yes, a while ago I cleaned up & deleted all my forks on github. How about we close this PR and you just make a new one with your simplified version?

One might separate the keywords, but in most IDE's I have seen they are all highlighted the same, so I guess that's more of a nice to have rather than a necessity.

No string prefixes that I know of.

Periods at the end of statements may have whitespaces before and after them.

Thanks for your help! :)

@pyrmont
Copy link
Contributor

pyrmont commented Jun 19, 2019

@raphaelhittich Sure thing. I'm sorry you won't get the credit for the commit :(

@pyrmont pyrmont dismissed their stale review June 19, 2019 07:03

This PR is being replaced

@ghost
Copy link
Author

ghost commented Jun 19, 2019

@pyrmont No worries, getting the lexer in is all that matters! :)

@ghost ghost closed this Jun 19, 2019
@pyrmont pyrmont mentioned this pull request Jun 19, 2019
@pyrmont pyrmont removed the author-action The PR has been reviewed but action by the author is needed label Aug 5, 2019
This pull request was closed.
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.

None yet

2 participants