Skip to content

fixes #4 update grammar to the latest spec #5

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

Merged
merged 3 commits into from
Mar 4, 2021

Conversation

marcellourbani
Copy link
Contributor

@larshp
This fits the bill of adjusting to the new format, but the output isn't great:
image
i.e. CLASS is not a keyword, comments are not dealt properly,...

To fix that I'll need to actually fix the grammar, possibly based on Marc's file above.
But I think it deserves its own PR

@joshgoebel
Copy link
Member

Are you sure the grammar is loaded properly? It seems to have support for comments...

@larshp
Copy link
Member

larshp commented Mar 3, 2021

heh, I dont exactly remember how things work 😄

@marcellourbani what triggered the above output? highlight js has some language auto-detection, did it detect a different language?

we need to setup some kind of automatic testing #3, some of the other repos in https://github.com/highlightjs probably got something we can borrow

@marcellourbani
Copy link
Contributor Author

marcellourbani commented Mar 3, 2021

I'm pretty confident: if I change the keywords the highlighting changes.
I think the begin condition is problematic, as contains a ^ (which is correct, * at the beginning of the line starts a comment, in other positions is either a multiplication or a syntax error)

    	{
    		className: 'comment',
    		begin: '^[*]',
    		relevance: 0,
    		end: '\n'
    	}

It does need fixing, just not sure the fix belongs to this PR

The html rendered above looks like this:

<html>
  <head>
    <link
      rel="stylesheet"
      href="https://cdnjs.cloudflare.com/ajax/libs/highlight.js/10.5.0/styles/default.min.css"
    />
    <script src="https://cdnjs.cloudflare.com/ajax/libs/highlight.js/10.5.0/highlight.min.js"></script>
    <script
      type="text/javascript"
      charset="UTF-8"
      src="../dist/abap.min.js"
    ></script>
    <script type="text/javascript">
      hljs.initHighlightingOnLoad()
    </script>
  </head>
  <body>
    <pre><code class="abap">
      REPORT  z_highlight_js.
      *&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&
      * Purpose:
      * ~~~~~~~~
      * This reports illustrates how an ABAP program looks in Highlight.js
      *&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&
      
      DATA: gt_sflight      TYPE sflight OCCURS 0,      " Output-Table
            gt_fieldcatalog TYPE lvc_t_fcat,
            ok_code LIKE sy-ucomm,
            save_ok LIKE sy-ucomm.           " OK-Code
      
      *###############################################################
      * LOCAL CLASSES
      *###############################################################
      *~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      * §4.Define a class for a data object to exchange data
      *    within ALV Control when using the drag and drop operation.
      CLASS LCL_DRAGDROPOBJ DEFINITION.
        PUBLIC SECTION.
          DATA: cps_sflight TYPE sflight,
                cp_node_text TYPE lvc_value,
                cp_node_key TYPE lvc_nkey.
      
      ENDCLASS.
      
      START-OF-SELECTION.
        WRITE 'END OF TEST'.
      
      END-OF-SELECTION.      
    </code></pre>
  </body>
</html>

@larshp
Copy link
Member

larshp commented Mar 3, 2021

the new dist/abap.min.js file, its a generated file? in that case we need to update the readme how to generate it and possibly add scripts in package.json

@marcellourbani
Copy link
Contributor Author

That's done by the main package, I will add a note about it.
Don't like the process one bit, but will document what it is

@marcellourbani
Copy link
Contributor Author

@joshgoebel
Ok, the comments work fine if the text is not indented.
image
grammar still needs fixing for the missing keywords like CLASS and ENDCLASS

@joshgoebel
Copy link
Member

Ok, the comments work fine if the text is not indented.

Yeah, the ^[*] rule would need to be updated to allow for potential whitespace... :-)

Don't like the process one bit, but will document what it is

You're welcome to generate dist in a different fashion if you think it'd be simpler... but there are other (testing/security) benefits to the current recommended setup as well.

@marcellourbani
Copy link
Contributor Author

@larshp I added the build instructions in README
@joshgoebel what I would like is a build command to run in my package.json
From what I understand of the tooling, as of today this would require:

  • clone the main repo
  • copy this in its extra folder
  • download modules
  • build
  • copy the dist folder
  • cleanup

Which is way more complex than what I feel like doing for this
I'm ok updating manually as we barely ever touch it

@larshp
Copy link
Member

larshp commented Mar 4, 2021

@marcellourbani thanks, merging

@larshp larshp merged commit fd84bcf into highlightjs:master Mar 4, 2021
@joshgoebel
Copy link
Member

joshgoebel commented Mar 4, 2021

Which is way more complex than what I feel like doing for this
I'm ok updating manually as we barely ever touch it

Yeah, it could be simpler, but after it's setup it's not much effort at all. Also, I've always started with the assumption that grammar authors are already building us - you need to in order to debug and test your grammar with the developer tools we provide (which are pretty helpful). Is that a faulty assumption?

The easiest way to test and debug a grammar is to build your grammar INSIDE our library.

what I would like is a build command to run in my package.json

Once you had the structure setup once I'd think a bash script that simply cd ../.. && build && cd extra/your_repo would suffice, no? I'm not saying that's perfect, but wouldn't that mostly work?

@larshp larshp mentioned this pull request Mar 4, 2021
@marcellourbani
Copy link
Contributor Author

@joshgoebel
Ok. Don't really like it but see no reason to make a fuss

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