Changed parsing library to flexmark#245
Conversation
|
it would be really great If one of you can take a look at this PR. We would really like to include it in release 0.4.1 - #231 (comment) |
martinvw
left a comment
martinvw
left a comment
There was a problem hiding this comment.
Thanks, I added some minor comments looks better great that so much code could be removed :-)
| <artifactId>commonmark</artifactId> | ||
| <version>${commonmark.version}</version> | ||
| </dependency> | ||
| <dependency> |
There was a problem hiding this comment.
Please format consistently it looks like a mixture of tabs and spaces.
| dispatcher.fireFileFinished(filePath); | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Why would you add this whitespace?
| * info</a> | ||
| * | ||
| * @author Erdoan Hadzhiyusein - Initial contribution | ||
| * @author Lyubomir Papazov - Change the Markdown parser to flexmark |
There was a problem hiding this comment.
The author-tags and signed off seem inconsistent
There was a problem hiding this comment.
Yes, actually @lpapazow did the changes on the checks and he's the rightful author. He's been working on a new binding and he hasn't had time to update this PR. Because of #244 , we decided that it would be a good idea to include this check in the new release, so I added a new test and fixed some comments from @dstivanov.
There was a problem hiding this comment.
AFAIK the also-by should then be added to the commit message, see https://docs.openhab.org/developers/contributing/contributing.html#sign-your-work
|
|
||
| MutableDataSet options = new MutableDataSet(); | ||
| //By setting this option to true, the parser provides line numbers in the original markdown text for each node | ||
| options.set(Parser.TRACK_DOCUMENT_LINES, true); |
There was a problem hiding this comment.
Hooray, so we do loose some of that flaky code
| // The code block is not the first line, and the previous line is not empty | ||
| if (zeroBasedStartLineNumber == 0 || !StringUtils.isBlank(fileText.get(zeroBasedStartLineNumber - 1))) { | ||
| // log the one-based line number | ||
| callback.log(zeroBasedStartLineNumber + 1, EMPTY_LINE_BEFORE_CODE_MSG); |
There was a problem hiding this comment.
I don't know whether you control the callback, but maybe you should do the adding of one there, it's too easy to forget one.
There was a problem hiding this comment.
Great idea! Thanks!
| @Test | ||
| public void testEmptyLineBeforeList() throws Exception { | ||
| String[] expectedMessages = generateExpectedMessages(1, "The line before a Markdown list must be empty."); | ||
| String[] expectedMessages = generateExpectedMessages(2, "The line before a Markdown list must be empty."); |
There was a problem hiding this comment.
Why do only some of the linenumbers change is that a bug or a feature?
There was a problem hiding this comment.
It is not a bug. After changing the parsing library and adding more tests, the logging in some cases seemed inconsistent, so we changed where the warning message should be placed.
Signed-off-by: Kristina S. Simova <kssimovaa@gmail.com>
kssimova
force-pushed
the
mardown-flexmark-new
branch
from
18bb0e4 to
755071c
Compare
|
LGTM |
|
Included in 0.4.1. |
4 participants
Fix for issues #205 and #244
Signed-off-by: Kristina S. Simova kssimovaa@gmail.com