strict comment syntax - Githubissues

concordion / concordion-excel-extension

Allows Concordion specifications to be in Excel format rather than HTML

Other

9 stars 12 forks source link

strict comment syntax #16

Open vgheo opened 5 years ago

vgheo commented 5 years ago

The current implementation of the parser for excel comments is a handcrafted parser that tolerates (ignores) free-text comments mixed with concordion syntax.

As a consequence, errors in the excel/concordion comments propagate to the intermediate html and are reported as errors at that level, with no traceability to the excel source location. This makes debugging of syntax issues very tedious. Examples:

accidentally introduced a similar-but-different character for single-quote
have two successive single-quotes that look identical to double-quote
(i bet users can come with loads of other examples... )

As an enhancement, the following is proposed

introduce a grammar-based parser for excel/concordion comments ( eg using https://www.antlr.org/ )
extend the @Extension annotation such that to allow the user to specify whether to use the "strict" comment parser (with better diagnosis) or the "lax" parser (current one - tolerant to freetext)

vgheo commented 5 years ago

Survey of parsers with Java: https://dzone.com/articles/parsing-in-java-part-2-diving-into-cfg-parsers https://dzone.com/articles/parsing-in-java-part-3-diving-into-peg-parsers

robmoffat commented 5 years ago

I think you’re right that error handling is less than perfect.

The problem is that the conversion leaves none of the context of where the errors exist in the spreadsheet. Is that correct? I’m a bit hazy on this now as I haven’t used it in a while

I’m not entirely sure how using a grammar would fix this?

vgheo commented 5 years ago

I see several problems.

Considering all errors that can occur in a concordion/excel specification, these can be categorized as follows: A. excel comment syntax errors

eg. mismatched quotes
(current issue) - these can be detected by a parser

B. excel to html translation errors

eg: (table) statement outside of an excel table
(issue #15) - these are detected by the extension and should be reported including excel location information

C. html syntax errors

I think these should never occur, provided parser at point A is strict enough

D. Concordion execution errors

eg: calling non-existent method
When coming from excel, these should contain infromation about the excel source location ( eg sheet/cell)
- This is the problem that you are referring to
- a variant of #15, but solution spans the extension and core concordion

I could consider contributing a solution, if you agree in principle that the enhacement makes sense and maybe provide some guidance.

robmoffat commented 5 years ago

This is excellent analysis, thanks.

Regarding (A), you could go down that route, but it might actually be less effort to just ruggedise the existing code. The coverage isn't wonderful at the moment:

(B) we discussed in #15 the exception handing (and debuggability of the html generally) could be improved by included information about where the HTML came from. Would that help with (D) too?