highcharts-for-python / highcharts-core

Python wrapper for the Highcharts Core JavaScript library
https://www.highcharts.com/integrations/python
Other
58 stars 13 forks source link

JS literal containing `+` for string concatenation breaks `from_js_literal()` #185

Closed JulienBacquart closed 4 months ago

JulienBacquart commented 4 months ago

Trying to reproduce Stacked percentage column

options_as_str = """{
    chart: {
        type: 'column'
    },
    title: {
        text: 'Domestic passenger transport by mode of transport, Norway',
        align: 'left'
    },
    subtitle: {
        text: 'Source: <a href="https://www.ssb.no/transport-og-reiseliv/landtransport/statistikk/innenlandsk-transport">SSB</a>',
        align: 'left'
    },
    xAxis: {
        categories: ['2019', '2020', '2021']
    },
    yAxis: {
        min: 0,
        title: {
            text: 'Percent'
        }
    },
    tooltip: {
        pointFormat: 
            '<span style="color:{series.color}">{series.name}</span>' +
            ': <b>{point.y}</b> ({point.percentage:.0f}%)<br/>',
            // '<span style="color:{series.color}">{series.name}</span>: <b>{point.y}</b> ({point.percentage:.0f}%)<br/>',
        shared: true
    },
    plotOptions: {
        column: {
            stacking: 'percent',
            dataLabels: {
                enabled: true,
                format: '{point.percentage:.0f}%'
            }
        }
    },
    series: [{
        name: 'Road',
        data: [434, 290, 307]
    }, {
        name: 'Rail',
        data: [272, 153, 156]
    }, {
        name: 'Air',
        data: [13, 7, 8]
    }, {
        name: 'Sea',
        data: [55, 35, 41]
    }]
}"""

options = HighchartsOptions.from_js_literal(options_as_str)
bar_chart = Chart.from_options(options)
bar_chart.display()

It seems the + is often used in the JS examples to break lines too long, but it breaks the from_js_literal() function.

If we look at the parse tree generated by Esprima

-
#6
type: Property
-
key
type: Identifier
name: tooltip
computed: false
-
value
    type: ObjectExpression
    -
    properties
    -
    #1
    type: Property
    -
    key
    type: Identifier
    name: pointFormat
    computed: false
    -
    value
        type: BinaryExpression
        operator: +
        -
        left
        type: Literal
        value: <span style="color:{series.color}">{series.name}</span>
        raw: '<span style="color:{series.color}">{series.name}</span>'
        -
        right
        type: Literal
        value: : <b>{point.y}</b> ({point.percentage:.0f}%)<br/>
        raw: ': <b>{point.y}</b> ({point.percentage:.0f}%)<br/>'
        kind: init
        method: false
        shorthand: false

We can see we generate a BinaryExpression which is not a value type covered by js_literal_functions().

A possible solution could look something like that:

    ...
    elif property_definition.value.type == 'BinaryExpression':
        property_value = property_definition.value
        operator = property_value.operator
        left, right = property_value.left, property_value.right
        left_type, right_type = left.type, right.type

        if (left_type not in ['Literal']) or (right_type not in ['Literal']):
            raise errors.HighchartsParseError(f'unable to find two Literal values within'
                                              f'a Binary expression. Found: '
                                              f'{left_type, right_type}')

        left_value, right_value = left.value, right.value

        if checkers.is_string(left_value) and checkers.is_string(right_value):
            if operator == '+':
                return left_value + right_value
            elif ...:
                 ...
        else:
            ...

If you want to open the can of worm of all valid combinaisons of types and binary operations, so we could write thing like min : 0.8 * 100, or text: 'Domestic passenger for the year ' + 2012,, is for you to decide.

hcpchris commented 4 months ago

Thanks, @JulienBacquart : This is a great catch. And I definitely appreciate the potential solution you propose. Let me give this one a little more thought - on the one hand, you're absolutely right that supporting the different type/binary operations is a can of worms...on the other hand, if we want to keep the promise of full support we probably should. Hmm. It's a bit of a conundrum. Let me give it a bit of thought.

In either case, supporting the + should be something that gets tackled in the next release.

JulienBacquart commented 4 months ago

Implementing completely Binary Expression would mean implementing all the following operators and all the possible combinaison of types possibles:

operator: 'instanceof' | 'in' | '+' | '-' | '*' | '/' | '%' | '**' |
        '|' | '^' | '&' | '==' | '!=' | '===' | '!==' |
        '<' | '>' | '<=' | '<<' | '>>' | '>>>';

At this point you are re-implementing a whole javascript interpreter. And I don't know much JS, but even I know that you can write some pretty cursed expression in JS because of the casting never failing:

"b" + "a" + +"a" + "a"; // -> 'baNaNa'