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

Correct for Fortran literal number #1934

Closed
wants to merge 3 commits into from
Closed

Conversation

kookma
Copy link

@kookma kookma commented Dec 25, 2018

In modern fortran you can define the kind of intrinsic data type. It means you can define additional intrinsic data type. When you do so, for literal constants you have to use the kind name attached to literal consonant using under score (like a prefix _newtype).
Example:
a = 12.23_dp

How the current code works

When using the current highlight.js to highlight a Fortran code contains such named constant, highlight.js fails to do the job! It highlights the remaining code (from the position of named constant) in a wrong way!
current_bug

Note to line 4. The literal number has a _DBL suffix and it makes the current code breaks. The remaining highlighting is wrong.

The Corrected code

The below image shows the corrected code output. As you can see the literal constant is highlighted correctly and there is no more error in the following texts.

corrected_code

Note to line four and following lines. The literal constants are highlighted correctly. Also note to line 5 the WRITE keyword in two image.

In modern fortran you can set the kind of intrinsic data type. It means you can define additional intrinsic data type. When you do so, for literal constants you have to use the kind name attached to literal consonant using under score.
Example:
 a = 12.23_dp
Copy link
Collaborator

@egor-rogov egor-rogov left a comment

Choose a reason for hiding this comment

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

Hi @kookma
Unfortunately tests are failing now. You can look right into Travis log (https://travis-ci.org/highlightjs/highlight.js/jobs/472075275) or run tests locally (the process is described here: https://highlightjs.readthedocs.io/en/latest/building-testing.html).

As I can see, the difference is the following:

  1. Plus/minus sign before a number is not expected to be a part of that number. For example, -.23 is expected to be -<span class="hljs-number">.23</span> but was highlighted as <span class="hljs-number">-.23</span>.

  2. A number can be ended with dot, which is a part of that number. For example, 1. expected to be <span class="hljs-number">1.</span>. But now it is highlighted as <span class="hljs-number">1</span>..

You should address these issues in the regexp, and also add a test (or couple of tests) for the introduced changes. The tests are in test/markup/fortran/ catalog.

@kookma
Copy link
Author

kookma commented Dec 26, 2018

@egor-rogov
I will check and correct the error. By the ay I checked other languages in when we write ( 12 - 13) negative sign is not part of number but for (-.12 + 13) the first sign is part of the number!

I understood you expect to remove the minus sign in the second example also! Am I right?

@egor-rogov
Copy link
Collaborator

@kookma
I think it's more about aesthetics then about formal grammar. Numbers are usually highlighted with some color while arithmetic operations are not. If you include unary minus to a number, you'll end up with differently colored minuses in expressions. I'd personally prefer consistently looking expressions, as it is implemented now.

@kookma
Copy link
Author

kookma commented Dec 26, 2018

@egor-rogov
I revised the fortran.js. Now the last period is part of number and starting plus/minus signs are not.

I will add some more tests and examples

@kookma
Copy link
Author

kookma commented Dec 26, 2018

@egor-rogov
Please let me know if it fine now. I have added many tests.

fortran-highlight-test

@egor-rogov
Copy link
Collaborator

@kookma
Probably you did't push tests? I can't see them in this PR.
Also, why change className to cN etc?

@kookma
Copy link
Author

kookma commented Dec 27, 2018

@egor-rogov
It seems something goes wrong! Can I delete this PR and create a new?
I am not very familiar with github system.

By the way I did not changed the cN class, I just changed the content of regexp string. Compare this with the original file in master branch. It seems I did something wrong during commit on myside

@egor-rogov
Copy link
Collaborator

@kookma Sure you can create a new PR.

@marcoscaceres
Copy link
Contributor

Closing based on comments above. Looking forward to a new pr.

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

3 participants