Add ISBL lexer #891

MedvedTMN · 2018-04-03T12:07:53Z

Add new lexer:

ISBL built-in language DIRECTUM
Add New theme:
ISBL editor light

Add new lexer: - ISBL built-in language DIRECTUM Add New theme: - ISBL editor light

MedvedTMN · 2018-04-03T15:06:12Z

I did not have errors for Ruby 2.5.1. What to do?

dblessing · 2018-04-03T17:27:56Z

It may have been a transient failure. I restarted the job to see what happens.

MedvedTMN · 2018-04-04T06:46:09Z

Problem still exists

- add isbl_editor_dark theme - fixed isbl_editor_light theme

dblessing · 2018-04-10T20:03:03Z

This may have been related to #892. Rebase against master and see if that fixes things.

pyrmont · 2019-08-01T17:58:15Z

@MedvedTMN Thanks for submitting the PR and I'm sorry it's taken so long to get this addressed. I joined the maintainers group a couple of months ago and have been working backwards chronologically through the outstanding PRs which is why I hadn't got to yours yet :(

I realise that @vidarh in his review had identified this as having only minor issues but I'm afraid that's not my conclusion. There are a couple of problems:

The architecture of this lexer results in a large number of String objects being added to Rouge. At this stage Rouge works by loading all lexers into memory. Running rake check:memory on master and on this branch shows that this lexer would lead to an increase in allocated memory of 1MB and to retained memory of 400KB. That's a significant amount compared to other lexers. Looking at the lexer, the vast majority of strings are constants. Does ISBL differentiate between constants and non-constants by capital letters? If it did, you could get rid of all these constants by merely highlighting any capitalised word as a constant.

If including all of these strings is inescapable, we would be looking to do two things. First, the strings should be put behind memoised class methods. You currently store these all as local variables which require allocation at the time the class is read. Using class methods can delay that allocation. A memoised class method looks like the following:

def self.some_identifier
  @some_identifier ||= %w(...)
end

Second, these strings can be stored in a separate file and then brought in by the memoised method when needed. You can see how this would work by looking at the Gherkin lexer:

rouge/lib/rouge/lexers/gherkin.rb

Lines 20 to 24 in 7836be9

    
           # self-modifying method that loads the keywords file 
        
           def self.keywords 
        
             load File.join(__dir__, 'gherkin/keywords.rb') 
        
             keywords 
        
           end

The file that is loaded by the Gherkin lexer is generated by a Rake task. You would need to create a similar task as part of the PR submission for ISBL.

The lexer is missing the frozen_string_literal pragma we use for all files in Rouge.
The lexer is called ISBL but the description suggests that the name of the language is DIRECTUM. Which name is the name of the language? If it is ISBL, the name of the lexer should be ISBL rather than Isbl.
The lexer contains inconsistent usage of whitespace for indenting. Rouge follows common Ruby practices and indents with two spaces. Please update the lexer accordingly.
Since the lexer was originally submitted, we have made RuboCop checks part of the test suite (you can run this at the command line by typing rake). To avoid RuboCop's ambiguous regular expression warnings, the parameter passed to rule should be prefaced with %r and a delimiter. The simplest delimiter is / such that rule /.../ becomes rule %r/.../.
We generally prefer using \b rather than lookaheads in patterns. I realise that ISBL, due to its provenance, may be used by users who will write identifiers with Cyrillic characters. Does this mean you can't use \b?
The PR includes two themes. I would suggest these be removed. I apologise for sounding a bit harsh but these appear to be primarily (if not only) of interest to the small subset of users that would want to highlight ISBL. Rouge's suggested approach for those situations is for users to include their own theming files separately in their projects.

Sorry you've had to wait so long only to then be hit with such a long list of suggested changes. I'll try to be as responsive as I can be to any follow-up questions you have.

pyrmont · 2019-08-02T11:44:14Z

@MedvedTMN Do you want it close this PR? It looks like perhaps you're planning to create your own fork?

MedvedTMN · 2019-08-02T12:09:09Z

No, I do not want to close this PR. I just wanted to update the local project files to correct your comments. Am I doing something wrong?

ashmaroli · 2019-08-02T12:28:24Z

Looks like @MedvedTMN changed the line-endings on all files and committed them..

ashmaroli · 2019-09-19T14:43:38Z

@MedvedTMN May I know what OS you're working on..? It looks like you altered the line endings of numerous files.

MedvedTMN · 2019-09-19T14:45:56Z

@MedvedTMN May I know what OS you're working on..? It looks like you altered the line endings of numerous files.

Windows 10

MedvedTMN · 2019-09-19T14:47:38Z

How can I fix it?

ashmaroli · 2019-09-19T15:09:29Z

@MedvedTMN Fixing this is perhaps too much for you (I'm assuming that you're not an advanced Git user). But you may want to read through the following documentation for future contributions:
https://help.github.com/en/articles/configuring-git-to-handle-line-endings

I'll try to fix this for you via a PR to your branch some time later..

MedvedTMN · 2019-09-19T15:21:14Z

I'll try to fix this for you via a PR to your branch some time later..

Thank you very much for your help

pyrmont

Thanks for the work on getting this ready to merge. It's looking good!

Could I ask you to have a look at reducing the visual sample so that it's not as duplicative? The visual sample doesn't need to be the absolute minimum required to test lexer's rules but for maintainability reasons, it's helpful to have it be more on the minimal side.

If that can be cut down a little, we'll be ready to merge.

ashmaroli · 2020-04-01T08:59:04Z

@MedvedTMN Why have trailing whitespace been added to the sample file in the latest commit..?

# Conflicts: # spec/visual/samples/isbl

This reverts commit 93261c4. # Conflicts: # spec/visual/samples/isbl

pyrmont · 2020-04-02T14:47:41Z

@MedvedTMN Thanks for fixing the trailing whitespace issue; however, you've added back in all the extraneous stuff in the visual sample you removed earlier. Can you delete that stuff again?

MedvedTMN · 2020-04-02T14:54:42Z

The current example most fully demonstrates the capabilities of the language. If the example is too large, I can shorten it. Or do you need to remove any specific extraneous stuff?

pyrmont · 2020-04-02T14:56:41Z

@MedvedTMN The code added back by this commit is the extraneous stuff I'm referring to.

MedvedTMN · 2020-04-02T14:58:40Z

What is wrong with this code?

pyrmont · 2020-04-02T15:04:39Z

@MedvedTMN Did you look at the link? All the code you added back in isn't necessary, is it? The point of the visual sample is to test the rules in the lexer. I don't see why a lexer that's 97 lines of code total needs a visual sample that's almost 300 lines.

pyrmont · 2020-04-02T15:07:45Z

@MedvedTMN To pick one thing as illustrative, you have multiple instances of multiline comments. One or two would suffice. I'm not familiar with this language so it's hard for me to identify definitively what's duplicative but it looks to me like there are many instances like this. The visual sample appears to be a copy and paste from existing code. A more minimal sample that better targeted the rules of the lexer would make this easier to maintain.

MedvedTMN · 2020-04-02T15:40:23Z

it's all right?

pyrmont · 2020-04-02T18:52:46Z

spec/visual/samples/isbl

+// Types
+type : IApplication
+type : .custom_type
+
+// Operators
+result = 1 + 1 / 2
+
+// Numbers
+integer = 5
+float = 5.5
+
+// Strings
+double_quotes = "An example"
+single_quotes = 'An example'


@MedvedTMN Is this correct syntax for ISBL? Looking quickly at the rules, it looked to me like .custom_type should be lexed as a type but that wasn't what happened. Could you have a look?

No, this is the wrong type definition for the ISBL variable. The following definition will be considered correct:
VariableName: ObjectType|IReference[.ReferenceType]|IEDocument[.CardType][ = Expression]
This is how this code looks in the built-in code editor:

Cool. Can you update the visual sample so that it's correct?

edit this part:
// Types
type : IApplication
type : .custom_type

or this:
Employees : IReference.РАБ = CreateReference(EMPLOYEES_REFERENCE; ArrayOf("Пользователь"; SYSREQ_STATE); FALSE)

pyrmont · 2020-04-03T18:22:45Z

@MedvedTMN I realise this took a long time but we finally got there in the end :) This will be part of the next release of Rouge. The gem is scheduled to be pushed on Tuesday 14 April 🎉

This commit adds a lexer for ISBL.

Add ISBL lexer

84d9333

Add new lexer: - ISBL built-in language DIRECTUM Add New theme: - ISBL editor light

Change theme name

ca20cb4

MedvedTMN added 2 commits April 9, 2018 13:24

Fixed a problem with multi-line strings

be99ad8

fixed isbl themes

1472c11

- add isbl_editor_dark theme - fixed isbl_editor_light theme

Fixed isbl_editor_dark theme

0d1adf8

vidarh mentioned this pull request Jan 17, 2019

Cleaning up pull requests - Reviewed lexer PRs #1063

Closed

pyrmont added the needs-review The PR needs to be reviewed label Jul 18, 2019

Merge branch 'master' into master

2424f3e

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 Aug 1, 2019

Syncing a fork

a906ddf

MedvedTMN added 3 commits August 6, 2019 17:28

resolve conflicts

5c261b4

Merge branch 'rouge-ruby-master'

c16b4a6

remove themes

a1416c4

pyrmont self-assigned this Aug 26, 2019

correction after code review

ff181ca

Cleanup ISBL lexer submission

5726644

pyrmont added needs-review The PR needs to be reviewed and removed author-action The PR has been reviewed but action by the author is needed labels Mar 24, 2020

pyrmont suggested changes Mar 31, 2020

View reviewed changes

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 Mar 31, 2020

Change visual sample

a3900dc

MedvedTMN added 6 commits April 1, 2020 12:11

Change visual sample

b20e137

Delete trailing whitespaces

93261c4

Delete trailing whitespaces

61586ae

Merge branch 'master' of https://github.com/MedvedTMN/rouge

0413964

# Conflicts: # spec/visual/samples/isbl

Revert "Delete trailing whitespaces"

95f7c3f

This reverts commit 93261c4. # Conflicts: # spec/visual/samples/isbl

Delete trailing whitespaces

0fe5099

Shortened visual sample

fd52e0e

pyrmont added 2 commits April 3, 2020 03:41

Add targeted examples to visual sample

7cfb39e

Convert file format to UNIX

3741e0d

pyrmont suggested changes Apr 2, 2020

View reviewed changes

Update visual sample

e8da3f7

pyrmont approved these changes Apr 3, 2020

View reviewed changes

pyrmont merged commit f3fb856 into rouge-ruby:master Apr 3, 2020

pyrmont removed the author-action The PR has been reviewed but action by the author is needed label Apr 3, 2020

mattt pushed a commit to NSHipster/rouge that referenced this pull request May 21, 2020

Add ISBL lexer (rouge-ruby#891)

b99231f

This commit adds a lexer for ISBL.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ISBL lexer #891

Add ISBL lexer #891

MedvedTMN commented Apr 3, 2018

MedvedTMN commented Apr 3, 2018

dblessing commented Apr 3, 2018

MedvedTMN commented Apr 4, 2018

dblessing commented Apr 10, 2018

pyrmont commented Aug 1, 2019 •

edited

pyrmont commented Aug 2, 2019

MedvedTMN commented Aug 2, 2019

ashmaroli commented Aug 2, 2019

ashmaroli commented Sep 19, 2019

MedvedTMN commented Sep 19, 2019

MedvedTMN commented Sep 19, 2019

ashmaroli commented Sep 19, 2019

MedvedTMN commented Sep 19, 2019

pyrmont left a comment

ashmaroli commented Apr 1, 2020

pyrmont commented Apr 2, 2020

MedvedTMN commented Apr 2, 2020

pyrmont commented Apr 2, 2020

MedvedTMN commented Apr 2, 2020

pyrmont commented Apr 2, 2020

pyrmont commented Apr 2, 2020

MedvedTMN commented Apr 2, 2020

pyrmont Apr 2, 2020 •

edited

MedvedTMN Apr 2, 2020 •

edited

pyrmont Apr 2, 2020

MedvedTMN Apr 2, 2020

MedvedTMN Apr 2, 2020

pyrmont Apr 3, 2020

pyrmont commented Apr 3, 2020

Add ISBL lexer #891

Add ISBL lexer #891

Conversation

MedvedTMN commented Apr 3, 2018

MedvedTMN commented Apr 3, 2018

dblessing commented Apr 3, 2018

MedvedTMN commented Apr 4, 2018

dblessing commented Apr 10, 2018

pyrmont commented Aug 1, 2019 • edited

pyrmont commented Aug 2, 2019

MedvedTMN commented Aug 2, 2019

ashmaroli commented Aug 2, 2019

ashmaroli commented Sep 19, 2019

MedvedTMN commented Sep 19, 2019

MedvedTMN commented Sep 19, 2019

ashmaroli commented Sep 19, 2019

MedvedTMN commented Sep 19, 2019

pyrmont left a comment

Choose a reason for hiding this comment

ashmaroli commented Apr 1, 2020

pyrmont commented Apr 2, 2020

MedvedTMN commented Apr 2, 2020

pyrmont commented Apr 2, 2020

MedvedTMN commented Apr 2, 2020

pyrmont commented Apr 2, 2020

pyrmont commented Apr 2, 2020

MedvedTMN commented Apr 2, 2020

pyrmont Apr 2, 2020 • edited

Choose a reason for hiding this comment

MedvedTMN Apr 2, 2020 • edited

Choose a reason for hiding this comment

pyrmont Apr 2, 2020

Choose a reason for hiding this comment

MedvedTMN Apr 2, 2020

Choose a reason for hiding this comment

MedvedTMN Apr 2, 2020

Choose a reason for hiding this comment

pyrmont Apr 3, 2020

Choose a reason for hiding this comment

pyrmont commented Apr 3, 2020

pyrmont commented Aug 1, 2019 •

edited

pyrmont Apr 2, 2020 •

edited

MedvedTMN Apr 2, 2020 •

edited