From 09cefca96abe8df50922d88bd1f8fae5994fa951 Mon Sep 17 00:00:00 2001 From: Stefan Binder Date: Sat, 1 Apr 2023 08:51:50 +0000 Subject: [PATCH 01/37] Add comments to existing tests what the errors are in the chart specification. Relevant as sometimes only one is shown in the resulting error message --- tests/utils/test_schemapi.py | 51 ++++++++++++++++++++++-------------- 1 file changed, 32 insertions(+), 19 deletions(-) diff --git a/tests/utils/test_schemapi.py b/tests/utils/test_schemapi.py index 8bc11b09b..70601ab9e 100644 --- a/tests/utils/test_schemapi.py +++ b/tests/utils/test_schemapi.py @@ -392,7 +392,8 @@ def test_schema_validation_error(): assert the_err.message in message -def chart_example_layer(): +def chart_error_example_layer(): + # Error: Width is not a valid property of a VConcatChart points = ( alt.Chart(data.cars.url) .mark_point() @@ -404,7 +405,8 @@ def chart_example_layer(): return (points & points).properties(width=400) -def chart_example_hconcat(): +def chart_error_example_hconcat(): + # Error: Invalid value for title in Text source = data.cars() points = ( alt.Chart(source) @@ -417,14 +419,17 @@ def chart_example_hconcat(): text = ( alt.Chart(source) - .mark_text(align="right") + .mark_text() .encode(alt.Text("Horsepower:N", title=dict(text="Horsepower", align="right"))) ) return points | text -def chart_example_invalid_channel_and_condition(): +def chart_error_example_invalid_channel(): + # Error: invalidChannel is an invalid encoding channel. Condition is correct + # but is added below as in previous implementations of Altair this interfered + # with finding the invalidChannel error selection = alt.selection_point() return ( alt.Chart(data.barley()) @@ -437,7 +442,8 @@ def chart_example_invalid_channel_and_condition(): ) -def chart_example_invalid_y_option(): +def chart_error_example_invalid_y_option_value_unknown_x_option(): + # Error: Invalid Y option value "asdf" and unknown option "unknown" for X return ( alt.Chart(data.barley()) .mark_bar() @@ -448,7 +454,8 @@ def chart_example_invalid_y_option(): ) -def chart_example_invalid_y_option_value(): +def chart_error_example_invalid_y_option_value(): + # Error: Invalid Y option value "asdf" return ( alt.Chart(data.barley()) .mark_bar() @@ -459,7 +466,10 @@ def chart_example_invalid_y_option_value(): ) -def chart_example_invalid_y_option_value_with_condition(): +def chart_error_example_invalid_y_option_value_with_condition(): + # Error: Invalid Y option value "asdf". Condition is correct + # but is added below as in previous implementations of Altair this interfered + # with finding the invalidChannel error return ( alt.Chart(data.barley()) .mark_bar() @@ -471,15 +481,18 @@ def chart_example_invalid_y_option_value_with_condition(): ) -def chart_example_invalid_timeunit_value(): +def chart_error_example_invalid_timeunit_value(): + # Error: Invalid value for Angle.timeUnit return alt.Chart().encode(alt.Angle().timeUnit("invalid_value")) -def chart_example_invalid_sort_value(): +def chart_error_example_invalid_sort_value(): + # Error: Invalid value for Angle.sort return alt.Chart().encode(alt.Angle().sort("invalid_value")) -def chart_example_invalid_bandposition_value(): +def chart_error_example_invalid_bandposition_value(): + # Error: Invalid value for Text.bandPosition return ( alt.Chart(data.cars()) .mark_text(align="right") @@ -491,7 +504,7 @@ def chart_example_invalid_bandposition_value(): "chart_func, expected_error_message", [ ( - chart_example_invalid_y_option, + chart_error_example_invalid_y_option_value_unknown_x_option, inspect.cleandoc( r"""`X` has no parameter named 'unknown' @@ -505,7 +518,7 @@ def chart_example_invalid_bandposition_value(): ), ), ( - chart_example_layer, + chart_error_example_layer, inspect.cleandoc( r"""`VConcatChart` has no parameter named 'width' @@ -519,7 +532,7 @@ def chart_example_invalid_bandposition_value(): ), ), ( - chart_example_invalid_y_option_value, + chart_error_example_invalid_y_option_value, inspect.cleandoc( r"""'asdf' is an invalid value for `stack`: @@ -529,7 +542,7 @@ def chart_example_invalid_bandposition_value(): ), ), ( - chart_example_invalid_y_option_value_with_condition, + chart_error_example_invalid_y_option_value_with_condition, inspect.cleandoc( r"""'asdf' is an invalid value for `stack`: @@ -539,7 +552,7 @@ def chart_example_invalid_bandposition_value(): ), ), ( - chart_example_hconcat, + chart_error_example_hconcat, inspect.cleandoc( r"""'{'text': 'Horsepower', 'align': 'right'}' is an invalid value for `title`: @@ -548,7 +561,7 @@ def chart_example_invalid_bandposition_value(): ), ), ( - chart_example_invalid_channel_and_condition, + chart_error_example_invalid_channel, inspect.cleandoc( r"""`Encoding` has no parameter named 'invalidChannel' @@ -565,7 +578,7 @@ def chart_example_invalid_bandposition_value(): ), ), ( - chart_example_invalid_timeunit_value, + chart_error_example_invalid_timeunit_value, inspect.cleandoc( r"""'invalid_value' is an invalid value for `timeUnit`: @@ -576,7 +589,7 @@ def chart_example_invalid_bandposition_value(): ), ), ( - chart_example_invalid_sort_value, + chart_error_example_invalid_sort_value, # Previuosly, the line # "'invalid_value' is not of type 'array'" appeared multiple times in # the error message. This test should prevent a regression. @@ -587,7 +600,7 @@ def chart_example_invalid_bandposition_value(): ), ), ( - chart_example_invalid_bandposition_value, + chart_error_example_invalid_bandposition_value, inspect.cleandoc( r"""'4' is an invalid value for `bandPosition`: From dc6285ef8648808b04147d69e1d9fad8b1eefdcc Mon Sep 17 00:00:00 2001 From: Stefan Binder Date: Sat, 1 Apr 2023 09:29:17 +0000 Subject: [PATCH 02/37] Refactor validate_jsonschema into smaller functions --- altair/utils/schemapi.py | 23 +++++++++++++++-------- tools/schemapi/schemapi.py | 23 +++++++++++++++-------- 2 files changed, 30 insertions(+), 16 deletions(-) diff --git a/altair/utils/schemapi.py b/altair/utils/schemapi.py index 03b5fc1ee..362a0315f 100644 --- a/altair/utils/schemapi.py +++ b/altair/utils/schemapi.py @@ -47,6 +47,19 @@ def debug_mode(arg): def validate_jsonschema(spec, schema, rootschema=None): + errors = _get_errors_from_spec(spec, schema, rootschema=rootschema) + if errors: + errors = _subset_to_most_relevant_errors(errors) + main_error = errors[0] + # They can be used to craft more helpful error messages when this error + # is being converted to a SchemaValidationError + main_error._additional_errors = errors[1:] + raise main_error + + +def _get_errors_from_spec( + spec, schema, rootschema=None +) -> List[jsonschema.exceptions.ValidationError]: # We don't use jsonschema.validate as this would validate the schema itself. # Instead, we pass the schema directly to the validator class. This is done for # two reasons: The schema comes from Vega-Lite and is not based on the user @@ -69,16 +82,10 @@ def validate_jsonschema(spec, schema, rootschema=None): validator_kwargs["format_checker"] = validator_cls.FORMAT_CHECKER validator = validator_cls(schema, **validator_kwargs) errors = list(validator.iter_errors(spec)) - if errors: - errors = _get_most_relevant_errors(errors) - main_error = errors[0] - # They can be used to craft more helpful error messages when this error - # is being converted to a SchemaValidationError - main_error._additional_errors = errors[1:] - raise main_error + return errors -def _get_most_relevant_errors( +def _subset_to_most_relevant_errors( errors: Sequence[jsonschema.exceptions.ValidationError], ) -> List[jsonschema.exceptions.ValidationError]: if len(errors) == 0: diff --git a/tools/schemapi/schemapi.py b/tools/schemapi/schemapi.py index 799cf0b16..77538874b 100644 --- a/tools/schemapi/schemapi.py +++ b/tools/schemapi/schemapi.py @@ -45,6 +45,19 @@ def debug_mode(arg): def validate_jsonschema(spec, schema, rootschema=None): + errors = _get_errors_from_spec(spec, schema, rootschema=rootschema) + if errors: + errors = _subset_to_most_relevant_errors(errors) + main_error = errors[0] + # They can be used to craft more helpful error messages when this error + # is being converted to a SchemaValidationError + main_error._additional_errors = errors[1:] + raise main_error + + +def _get_errors_from_spec( + spec, schema, rootschema=None +) -> List[jsonschema.exceptions.ValidationError]: # We don't use jsonschema.validate as this would validate the schema itself. # Instead, we pass the schema directly to the validator class. This is done for # two reasons: The schema comes from Vega-Lite and is not based on the user @@ -67,16 +80,10 @@ def validate_jsonschema(spec, schema, rootschema=None): validator_kwargs["format_checker"] = validator_cls.FORMAT_CHECKER validator = validator_cls(schema, **validator_kwargs) errors = list(validator.iter_errors(spec)) - if errors: - errors = _get_most_relevant_errors(errors) - main_error = errors[0] - # They can be used to craft more helpful error messages when this error - # is being converted to a SchemaValidationError - main_error._additional_errors = errors[1:] - raise main_error + return errors -def _get_most_relevant_errors( +def _subset_to_most_relevant_errors( errors: Sequence[jsonschema.exceptions.ValidationError], ) -> List[jsonschema.exceptions.ValidationError]: if len(errors) == 0: From a1631a1b84dc6bd563fb03a12624044e323a6b24 Mon Sep 17 00:00:00 2001 From: Stefan Binder Date: Sun, 2 Apr 2023 15:45:24 +0000 Subject: [PATCH 03/37] Add new way of prioritizing errors. Group them by path to offending element before passing to SchemaValidationError to allow for better error messages to be created --- altair/utils/schemapi.py | 227 +++++++++++++++++++++++++---------- tests/utils/test_schemapi.py | 35 ++++-- tools/schemapi/schemapi.py | 227 +++++++++++++++++++++++++---------- 3 files changed, 351 insertions(+), 138 deletions(-) diff --git a/altair/utils/schemapi.py b/altair/utils/schemapi.py index 362a0315f..6a583b4ba 100644 --- a/altair/utils/schemapi.py +++ b/altair/utils/schemapi.py @@ -5,7 +5,7 @@ import inspect import json import textwrap -from typing import Any, Sequence, List, Dict, Optional +from typing import Any, Sequence, List, Dict, Optional, DefaultDict from itertools import zip_longest import jsonschema @@ -16,6 +16,9 @@ from altair import vegalite +ValidationErrorList = List[jsonschema.exceptions.ValidationError] +GroupedValidationErrors = Dict[str, ValidationErrorList] + # If DEBUG_MODE is True, then schema objects are converted to dict and # validated at creation time. This slows things down, particularly for @@ -46,20 +49,27 @@ def debug_mode(arg): DEBUG_MODE = original -def validate_jsonschema(spec, schema, rootschema=None): +def validate_jsonschema(spec, schema, rootschema=None, raise_error=True): errors = _get_errors_from_spec(spec, schema, rootschema=rootschema) if errors: - errors = _subset_to_most_relevant_errors(errors) - main_error = errors[0] + leaf_errors = _get_leaves_of_error_tree(errors) + grouped_errors = _group_errors_by_json_path(leaf_errors) + grouped_errors = _subset_to_most_specific_json_paths(grouped_errors) + grouped_errors = _deduplicate_errors(grouped_errors) + + # Nothing special about this first error but we need to choose one + # which can be raised + main_error = list(grouped_errors.values())[0][0] # They can be used to craft more helpful error messages when this error # is being converted to a SchemaValidationError - main_error._additional_errors = errors[1:] - raise main_error + main_error._all_errors = grouped_errors + if raise_error: + raise main_error + else: + return main_error -def _get_errors_from_spec( - spec, schema, rootschema=None -) -> List[jsonschema.exceptions.ValidationError]: +def _get_errors_from_spec(spec, schema, rootschema=None) -> ValidationErrorList: # We don't use jsonschema.validate as this would validate the schema itself. # Instead, we pass the schema directly to the validator class. This is done for # two reasons: The schema comes from Vega-Lite and is not based on the user @@ -85,61 +95,144 @@ def _get_errors_from_spec( return errors -def _subset_to_most_relevant_errors( - errors: Sequence[jsonschema.exceptions.ValidationError], -) -> List[jsonschema.exceptions.ValidationError]: - if len(errors) == 0: - return [] - - # Start from the first error on the top-level as we want to show - # an error message for one specific error only even if the chart - # specification might have multiple issues - top_level_error = errors[0] - - # Go to lowest level in schema where an error happened as these give - # the most relevant error messages. - lowest_level = top_level_error - while lowest_level.context: - lowest_level = lowest_level.context[0] - - most_relevant_errors = [] - if lowest_level.validator == "additionalProperties": - # For these errors, the message is already informative enough and the other - # errors on the lowest level are not helpful for a user but instead contain - # the same information in a more verbose way - most_relevant_errors = [lowest_level] - else: - parent = lowest_level.parent - if parent is None: - # In this case we are still at the top level and can return all errors - most_relevant_errors = list(errors) +def _group_errors_by_json_path( + errors: ValidationErrorList, +) -> GroupedValidationErrors: + errors_by_json_path = collections.defaultdict(list) + for err in errors: + errors_by_json_path[err.json_path].append(err) + return dict(errors_by_json_path) + + +def _get_leaves_of_error_tree( + errors: ValidationErrorList, +) -> ValidationErrorList: + leaves: ValidationErrorList = [] + for err in errors: + if err.context: + leaves.extend(_get_leaves_of_error_tree(err.context)) else: - # Use all errors of the lowest level out of which - # we can construct more informative error messages - most_relevant_errors = parent.context or [] - if lowest_level.validator == "enum": - # There might be other possible enums which are allowed, e.g. for - # the "timeUnit" property of the "Angle" encoding channel. These do not - # necessarily need to be in the same branch of this tree of errors that - # we traversed down to the lowest level. We therefore gather - # all enums in the leaves of the error tree. - enum_errors = _get_all_lowest_errors_with_validator( - top_level_error, validator="enum" - ) - # Remove errors which already exist in enum_errors - enum_errors = [ - err - for err in enum_errors - if err.message not in [e.message for e in most_relevant_errors] - ] - most_relevant_errors = most_relevant_errors + enum_errors + leaves.append(err) + return leaves + + +def _subset_to_most_specific_json_paths(errors_by_json_path: GroupedValidationErrors): + errors_by_json_path_specific: GroupedValidationErrors = {} + for json_path, errors in errors_by_json_path.items(): + if not _contained_at_start_of_one_of_other_values( + json_path, list(errors_by_json_path.keys()) + ): + errors_by_json_path_specific[json_path] = errors + return errors_by_json_path_specific + + +def _contained_at_start_of_one_of_other_values(x: str, values: Sequence[str]) -> bool: + # Does not count as "contained at start of other value" if the values are + # the same. These cases should be handled separately + return any(value.startswith(x) for value in values if x != value) + + +def _deduplicate_errors( + grouped_errors: GroupedValidationErrors, +) -> GroupedValidationErrors: + grouped_errors_deduplicated: GroupedValidationErrors = {} + for json_path, element_errors in grouped_errors.items(): + errors_by_validator = _group_errors_by_validator(element_errors) + + deduplication_functions = { + "enum": _deduplicate_enum_errors, + "additionalProperties": _deduplicate_additional_properties_errors, + } + deduplicated_errors: ValidationErrorList = [] + for validator, errors in errors_by_validator.items(): + deduplication_func = deduplication_functions.get(validator, None) + if deduplication_func is not None: + errors = deduplication_func(errors) + deduplicated_errors.extend(_deduplicate_by_message(errors)) + + if len(deduplicated_errors) > 1: + # Removes any ValidationError "'value' is a required property" as these + # errors are unlikely to be the relevant ones for the user. They come from + # validation against a schema definition where the output of `alt.value` + # would be valid. However, if a user uses `alt.value`, the `value` keyword + # is included automatically from that function and so it's unlikely + # that this was what the user intended if the keyword is not present + # in the first place. + deduplicated_errors = [ + err for err in deduplicated_errors if not _is_required_value_error(err) + ] + + grouped_errors_deduplicated[json_path] = deduplicated_errors + return grouped_errors_deduplicated + + +def _is_required_value_error(err: jsonschema.exceptions.ValidationError) -> bool: + return err.validator == "required" and err.validator_value == ["value"] + + +def _group_errors_by_validator(errors: ValidationErrorList) -> GroupedValidationErrors: + errors_by_validator: DefaultDict[ + str, ValidationErrorList + ] = collections.defaultdict(list) + for err in errors: + # Ignore mypy error as err.validator as it wrongly sees err.validator + # as of type Optional[Validator] instead of str + errors_by_validator[err.validator].append(err) # type: ignore[index] + return dict(errors_by_validator) + + +def _deduplicate_enum_errors(errors: ValidationErrorList) -> ValidationErrorList: + if len(errors) > 1: + # Deduplicate enum errors by removing the errors where the allowed values + # are a subset of another error. For example, if `enum` contains two errors + # and one has `validator_value` (i.e. accepted values) ["A", "B"] and the + # other one ["A", "B", "C"] then the first one is removed and the final + # `enum` list only contains the error with ["A", "B", "C"] + + # Values (and therefore `validator_value`) of an enum are always arrays, + # see https://json-schema.org/understanding-json-schema/reference/generic.html#enumerated-values + # which is why we can use join below + value_strings = [",".join(err.validator_value) for err in errors] + longest_enums: ValidationErrorList = [] + for value_str, err in zip(value_strings, errors): + if not _contained_at_start_of_one_of_other_values(value_str, value_strings): + longest_enums.append(err) + errors = longest_enums + return errors + + +def _deduplicate_additional_properties_errors( + errors: ValidationErrorList, +) -> ValidationErrorList: + if len(errors) > 1: + # If there are multiple additional property errors it usually means that + # the offending element was validated against multiple schemas and + # its parent is a common anyOf validator. + # The error messages produced from these cases are usually + # very similar and we just take the shortest one. For example, + # the following 3 errors are raised for the `unknown` channel option in + # `alt.X("variety", unknown=2)`: + # - "Additional properties are not allowed ('unknown' was unexpected)" + # - "Additional properties are not allowed ('field', 'unknown' were unexpected)" + # - "Additional properties are not allowed ('field', 'type', 'unknown' were unexpected)" + # The code below subsets this to only the first error of the three. + parent = errors[0].parent + # Test if all parent errors are the same anyOf error and only do + # the prioritization in these cases. Can't think of a chart spec where this + # would not be the case but still allow for it below to not break anything. + if ( + parent is not None + and parent.validator == "anyOf" + and all(err.parent is parent for err in errors[1:]) + ): + errors = [min(errors, key=lambda x: len(x.message))] + return errors - # This should never happen but might still be good to test for it as else - # the original error would just slip through without being raised - if len(most_relevant_errors) == 0: - raise Exception("Could not determine the most relevant errors") from errors[0] - return most_relevant_errors +def _deduplicate_by_message(errors: ValidationErrorList) -> ValidationErrorList: + # Deduplicate errors by message. This keeps the original order in case + # it was chosen intentionally + return list({e.message: e for e in errors}.values()) def _get_all_lowest_errors_with_validator( @@ -199,7 +292,7 @@ class SchemaValidationError(jsonschema.ValidationError): def __init__(self, obj, err): super(SchemaValidationError, self).__init__(**err._contents()) self.obj = obj - self._additional_errors = getattr(err, "_additional_errors", []) + self.errors = getattr(err, "_all_errors", {err.json_path: [err]}) @staticmethod def _format_params_as_table(param_dict_keys): @@ -305,7 +398,13 @@ def __str__(self): {message}""" - if self._additional_errors: + additional_errors = [ + err + for errors in self.errors.values() + for err in errors + if err is not list(self.errors.values())[0][0] + ] + if additional_errors: # Deduplicate error messages and only include them if they are # different then the main error message stored in self.message. # Using dict instead of set to keep the original order in case it was @@ -314,7 +413,7 @@ def __str__(self): dict.fromkeys( [ e.message - for e in self._additional_errors + for e in additional_errors if e.message != self.message ] ) diff --git a/tests/utils/test_schemapi.py b/tests/utils/test_schemapi.py index 70601ab9e..e9ba8c7d0 100644 --- a/tests/utils/test_schemapi.py +++ b/tests/utils/test_schemapi.py @@ -500,6 +500,11 @@ def chart_error_example_invalid_bandposition_value(): ) +def chart_error_invalid_type(): + # Error: Invalid value for type + return alt.Chart().encode(alt.X(type="unknown")) + + @pytest.mark.parametrize( "chart_func, expected_error_message", [ @@ -557,7 +562,8 @@ def chart_error_example_invalid_bandposition_value(): r"""'{'text': 'Horsepower', 'align': 'right'}' is an invalid value for `title`: {'text': 'Horsepower', 'align': 'right'} is not of type 'string' - {'text': 'Horsepower', 'align': 'right'} is not of type 'array'$""" + {'text': 'Horsepower', 'align': 'right'} is not of type 'array' + {'text': 'Horsepower', 'align': 'right'} is not of type 'null'$""" ), ), ( @@ -581,22 +587,25 @@ def chart_error_example_invalid_bandposition_value(): chart_error_example_invalid_timeunit_value, inspect.cleandoc( r"""'invalid_value' is an invalid value for `timeUnit`: - + 'invalid_value' is not one of \['year', 'quarter', 'month', 'week', 'day', 'dayofyear', 'date', 'hours', 'minutes', 'seconds', 'milliseconds'\] 'invalid_value' is not one of \['utcyear', 'utcquarter', 'utcmonth', 'utcweek', 'utcday', 'utcdayofyear', 'utcdate', 'utchours', 'utcminutes', 'utcseconds', 'utcmilliseconds'\] 'invalid_value' is not one of \['yearquarter', 'yearquartermonth', 'yearmonth', 'yearmonthdate', 'yearmonthdatehours', 'yearmonthdatehoursminutes', 'yearmonthdatehoursminutesseconds', 'yearweek', 'yearweekday', 'yearweekdayhours', 'yearweekdayhoursminutes', 'yearweekdayhoursminutesseconds', 'yeardayofyear', 'quartermonth', 'monthdate', 'monthdatehours', 'monthdatehoursminutes', 'monthdatehoursminutesseconds', 'weekday', 'weeksdayhours', 'weekdayhoursminutes', 'weekdayhoursminutesseconds', 'dayhours', 'dayhoursminutes', 'dayhoursminutesseconds', 'hoursminutes', 'hoursminutesseconds', 'minutesseconds', 'secondsmilliseconds'\] - 'invalid_value' is not one of \['utcyearquarter', 'utcyearquartermonth', 'utcyearmonth', 'utcyearmonthdate', 'utcyearmonthdatehours', 'utcyearmonthdatehoursminutes', 'utcyearmonthdatehoursminutesseconds', 'utcyearweek', 'utcyearweekday', 'utcyearweekdayhours', 'utcyearweekdayhoursminutes', 'utcyearweekdayhoursminutesseconds', 'utcyeardayofyear', 'utcquartermonth', 'utcmonthdate', 'utcmonthdatehours', 'utcmonthdatehoursminutes', 'utcmonthdatehoursminutesseconds', 'utcweekday', 'utcweeksdayhours', 'utcweekdayhoursminutes', 'utcweekdayhoursminutesseconds', 'utcdayhours', 'utcdayhoursminutes', 'utcdayhoursminutesseconds', 'utchoursminutes', 'utchoursminutesseconds', 'utcminutesseconds', 'utcsecondsmilliseconds'\]$""" + 'invalid_value' is not one of \['utcyearquarter', 'utcyearquartermonth', 'utcyearmonth', 'utcyearmonthdate', 'utcyearmonthdatehours', 'utcyearmonthdatehoursminutes', 'utcyearmonthdatehoursminutesseconds', 'utcyearweek', 'utcyearweekday', 'utcyearweekdayhours', 'utcyearweekdayhoursminutes', 'utcyearweekdayhoursminutesseconds', 'utcyeardayofyear', 'utcquartermonth', 'utcmonthdate', 'utcmonthdatehours', 'utcmonthdatehoursminutes', 'utcmonthdatehoursminutesseconds', 'utcweekday', 'utcweeksdayhours', 'utcweekdayhoursminutes', 'utcweekdayhoursminutesseconds', 'utcdayhours', 'utcdayhoursminutes', 'utcdayhoursminutesseconds', 'utchoursminutes', 'utchoursminutesseconds', 'utcminutesseconds', 'utcsecondsmilliseconds'\] + 'invalid_value' is not of type 'object'$""" ), ), ( chart_error_example_invalid_sort_value, - # Previuosly, the line - # "'invalid_value' is not of type 'array'" appeared multiple times in - # the error message. This test should prevent a regression. inspect.cleandoc( r"""'invalid_value' is an invalid value for `sort`: - 'invalid_value' is not of type 'array'$""" + 'invalid_value' is not of type 'array' + 'invalid_value' is not of type 'object' + 'invalid_value' is not of type 'null' + 'invalid_value' is not one of \['ascending', 'descending'\] + 'invalid_value' is not one of \['x', 'y', 'color', 'fill', 'stroke', 'strokeWidth', 'size', 'shape', 'fillOpacity', 'strokeOpacity', 'opacity', 'text'\] + 'invalid_value' is not one of \['-x', '-y', '-color', '-fill', '-stroke', '-strokeWidth', '-size', '-shape', '-fillOpacity', '-strokeOpacity', '-opacity', '-text'\]$""" ), ), ( @@ -604,9 +613,15 @@ def chart_error_example_invalid_bandposition_value(): inspect.cleandoc( r"""'4' is an invalid value for `bandPosition`: - '4' is not of type 'number' - Additional properties are not allowed \('field' was unexpected\) - Additional properties are not allowed \('bandPosition', 'field', 'type' were unexpected\)$""" + '4' is not of type 'number'$""" + ), + ), + ( + chart_error_invalid_type, + inspect.cleandoc( + r"""'unknown' is an invalid value for `type`: + + 'unknown' is not one of \['quantitative', 'ordinal', 'temporal', 'nominal', 'geojson'\]$""" ), ), ], diff --git a/tools/schemapi/schemapi.py b/tools/schemapi/schemapi.py index 77538874b..2b72b3bdf 100644 --- a/tools/schemapi/schemapi.py +++ b/tools/schemapi/schemapi.py @@ -3,7 +3,7 @@ import inspect import json import textwrap -from typing import Any, Sequence, List, Dict, Optional +from typing import Any, Sequence, List, Dict, Optional, DefaultDict from itertools import zip_longest import jsonschema @@ -14,6 +14,9 @@ from altair import vegalite +ValidationErrorList = List[jsonschema.exceptions.ValidationError] +GroupedValidationErrors = Dict[str, ValidationErrorList] + # If DEBUG_MODE is True, then schema objects are converted to dict and # validated at creation time. This slows things down, particularly for @@ -44,20 +47,27 @@ def debug_mode(arg): DEBUG_MODE = original -def validate_jsonschema(spec, schema, rootschema=None): +def validate_jsonschema(spec, schema, rootschema=None, raise_error=True): errors = _get_errors_from_spec(spec, schema, rootschema=rootschema) if errors: - errors = _subset_to_most_relevant_errors(errors) - main_error = errors[0] + leaf_errors = _get_leaves_of_error_tree(errors) + grouped_errors = _group_errors_by_json_path(leaf_errors) + grouped_errors = _subset_to_most_specific_json_paths(grouped_errors) + grouped_errors = _deduplicate_errors(grouped_errors) + + # Nothing special about this first error but we need to choose one + # which can be raised + main_error = list(grouped_errors.values())[0][0] # They can be used to craft more helpful error messages when this error # is being converted to a SchemaValidationError - main_error._additional_errors = errors[1:] - raise main_error + main_error._all_errors = grouped_errors + if raise_error: + raise main_error + else: + return main_error -def _get_errors_from_spec( - spec, schema, rootschema=None -) -> List[jsonschema.exceptions.ValidationError]: +def _get_errors_from_spec(spec, schema, rootschema=None) -> ValidationErrorList: # We don't use jsonschema.validate as this would validate the schema itself. # Instead, we pass the schema directly to the validator class. This is done for # two reasons: The schema comes from Vega-Lite and is not based on the user @@ -83,61 +93,144 @@ def _get_errors_from_spec( return errors -def _subset_to_most_relevant_errors( - errors: Sequence[jsonschema.exceptions.ValidationError], -) -> List[jsonschema.exceptions.ValidationError]: - if len(errors) == 0: - return [] - - # Start from the first error on the top-level as we want to show - # an error message for one specific error only even if the chart - # specification might have multiple issues - top_level_error = errors[0] - - # Go to lowest level in schema where an error happened as these give - # the most relevant error messages. - lowest_level = top_level_error - while lowest_level.context: - lowest_level = lowest_level.context[0] - - most_relevant_errors = [] - if lowest_level.validator == "additionalProperties": - # For these errors, the message is already informative enough and the other - # errors on the lowest level are not helpful for a user but instead contain - # the same information in a more verbose way - most_relevant_errors = [lowest_level] - else: - parent = lowest_level.parent - if parent is None: - # In this case we are still at the top level and can return all errors - most_relevant_errors = list(errors) +def _group_errors_by_json_path( + errors: ValidationErrorList, +) -> GroupedValidationErrors: + errors_by_json_path = collections.defaultdict(list) + for err in errors: + errors_by_json_path[err.json_path].append(err) + return dict(errors_by_json_path) + + +def _get_leaves_of_error_tree( + errors: ValidationErrorList, +) -> ValidationErrorList: + leaves: ValidationErrorList = [] + for err in errors: + if err.context: + leaves.extend(_get_leaves_of_error_tree(err.context)) else: - # Use all errors of the lowest level out of which - # we can construct more informative error messages - most_relevant_errors = parent.context or [] - if lowest_level.validator == "enum": - # There might be other possible enums which are allowed, e.g. for - # the "timeUnit" property of the "Angle" encoding channel. These do not - # necessarily need to be in the same branch of this tree of errors that - # we traversed down to the lowest level. We therefore gather - # all enums in the leaves of the error tree. - enum_errors = _get_all_lowest_errors_with_validator( - top_level_error, validator="enum" - ) - # Remove errors which already exist in enum_errors - enum_errors = [ - err - for err in enum_errors - if err.message not in [e.message for e in most_relevant_errors] - ] - most_relevant_errors = most_relevant_errors + enum_errors + leaves.append(err) + return leaves + + +def _subset_to_most_specific_json_paths(errors_by_json_path: GroupedValidationErrors): + errors_by_json_path_specific: GroupedValidationErrors = {} + for json_path, errors in errors_by_json_path.items(): + if not _contained_at_start_of_one_of_other_values( + json_path, list(errors_by_json_path.keys()) + ): + errors_by_json_path_specific[json_path] = errors + return errors_by_json_path_specific + + +def _contained_at_start_of_one_of_other_values(x: str, values: Sequence[str]) -> bool: + # Does not count as "contained at start of other value" if the values are + # the same. These cases should be handled separately + return any(value.startswith(x) for value in values if x != value) + + +def _deduplicate_errors( + grouped_errors: GroupedValidationErrors, +) -> GroupedValidationErrors: + grouped_errors_deduplicated: GroupedValidationErrors = {} + for json_path, element_errors in grouped_errors.items(): + errors_by_validator = _group_errors_by_validator(element_errors) + + deduplication_functions = { + "enum": _deduplicate_enum_errors, + "additionalProperties": _deduplicate_additional_properties_errors, + } + deduplicated_errors: ValidationErrorList = [] + for validator, errors in errors_by_validator.items(): + deduplication_func = deduplication_functions.get(validator, None) + if deduplication_func is not None: + errors = deduplication_func(errors) + deduplicated_errors.extend(_deduplicate_by_message(errors)) + + if len(deduplicated_errors) > 1: + # Removes any ValidationError "'value' is a required property" as these + # errors are unlikely to be the relevant ones for the user. They come from + # validation against a schema definition where the output of `alt.value` + # would be valid. However, if a user uses `alt.value`, the `value` keyword + # is included automatically from that function and so it's unlikely + # that this was what the user intended if the keyword is not present + # in the first place. + deduplicated_errors = [ + err for err in deduplicated_errors if not _is_required_value_error(err) + ] + + grouped_errors_deduplicated[json_path] = deduplicated_errors + return grouped_errors_deduplicated + + +def _is_required_value_error(err: jsonschema.exceptions.ValidationError) -> bool: + return err.validator == "required" and err.validator_value == ["value"] + + +def _group_errors_by_validator(errors: ValidationErrorList) -> GroupedValidationErrors: + errors_by_validator: DefaultDict[ + str, ValidationErrorList + ] = collections.defaultdict(list) + for err in errors: + # Ignore mypy error as err.validator as it wrongly sees err.validator + # as of type Optional[Validator] instead of str + errors_by_validator[err.validator].append(err) # type: ignore[index] + return dict(errors_by_validator) + + +def _deduplicate_enum_errors(errors: ValidationErrorList) -> ValidationErrorList: + if len(errors) > 1: + # Deduplicate enum errors by removing the errors where the allowed values + # are a subset of another error. For example, if `enum` contains two errors + # and one has `validator_value` (i.e. accepted values) ["A", "B"] and the + # other one ["A", "B", "C"] then the first one is removed and the final + # `enum` list only contains the error with ["A", "B", "C"] + + # Values (and therefore `validator_value`) of an enum are always arrays, + # see https://json-schema.org/understanding-json-schema/reference/generic.html#enumerated-values + # which is why we can use join below + value_strings = [",".join(err.validator_value) for err in errors] + longest_enums: ValidationErrorList = [] + for value_str, err in zip(value_strings, errors): + if not _contained_at_start_of_one_of_other_values(value_str, value_strings): + longest_enums.append(err) + errors = longest_enums + return errors + + +def _deduplicate_additional_properties_errors( + errors: ValidationErrorList, +) -> ValidationErrorList: + if len(errors) > 1: + # If there are multiple additional property errors it usually means that + # the offending element was validated against multiple schemas and + # its parent is a common anyOf validator. + # The error messages produced from these cases are usually + # very similar and we just take the shortest one. For example, + # the following 3 errors are raised for the `unknown` channel option in + # `alt.X("variety", unknown=2)`: + # - "Additional properties are not allowed ('unknown' was unexpected)" + # - "Additional properties are not allowed ('field', 'unknown' were unexpected)" + # - "Additional properties are not allowed ('field', 'type', 'unknown' were unexpected)" + # The code below subsets this to only the first error of the three. + parent = errors[0].parent + # Test if all parent errors are the same anyOf error and only do + # the prioritization in these cases. Can't think of a chart spec where this + # would not be the case but still allow for it below to not break anything. + if ( + parent is not None + and parent.validator == "anyOf" + and all(err.parent is parent for err in errors[1:]) + ): + errors = [min(errors, key=lambda x: len(x.message))] + return errors - # This should never happen but might still be good to test for it as else - # the original error would just slip through without being raised - if len(most_relevant_errors) == 0: - raise Exception("Could not determine the most relevant errors") from errors[0] - return most_relevant_errors +def _deduplicate_by_message(errors: ValidationErrorList) -> ValidationErrorList: + # Deduplicate errors by message. This keeps the original order in case + # it was chosen intentionally + return list({e.message: e for e in errors}.values()) def _get_all_lowest_errors_with_validator( @@ -197,7 +290,7 @@ class SchemaValidationError(jsonschema.ValidationError): def __init__(self, obj, err): super(SchemaValidationError, self).__init__(**err._contents()) self.obj = obj - self._additional_errors = getattr(err, "_additional_errors", []) + self.errors = getattr(err, "_all_errors", {err.json_path: [err]}) @staticmethod def _format_params_as_table(param_dict_keys): @@ -303,7 +396,13 @@ def __str__(self): {message}""" - if self._additional_errors: + additional_errors = [ + err + for errors in self.errors.values() + for err in errors + if err is not list(self.errors.values())[0][0] + ] + if additional_errors: # Deduplicate error messages and only include them if they are # different then the main error message stored in self.message. # Using dict instead of set to keep the original order in case it was @@ -312,7 +411,7 @@ def __str__(self): dict.fromkeys( [ e.message - for e in self._additional_errors + for e in additional_errors if e.message != self.message ] ) From 77290cfba1b6483429ae9c97302e1108e5eb7fc2 Mon Sep 17 00:00:00 2001 From: Stefan Binder Date: Fri, 7 Apr 2023 12:32:01 +0000 Subject: [PATCH 04/37] Remove whitespaces in blank line --- tests/utils/test_schemapi.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/utils/test_schemapi.py b/tests/utils/test_schemapi.py index 736d6316d..feb6f430b 100644 --- a/tests/utils/test_schemapi.py +++ b/tests/utils/test_schemapi.py @@ -590,7 +590,7 @@ def chart_error_invalid_type(): chart_error_example_invalid_timeunit_value, inspect.cleandoc( r"""'invalid_value' is an invalid value for `timeUnit`: - + 'invalid_value' is not one of \['year', 'quarter', 'month', 'week', 'day', 'dayofyear', 'date', 'hours', 'minutes', 'seconds', 'milliseconds'\] 'invalid_value' is not one of \['utcyear', 'utcquarter', 'utcmonth', 'utcweek', 'utcday', 'utcdayofyear', 'utcdate', 'utchours', 'utcminutes', 'utcseconds', 'utcmilliseconds'\] 'invalid_value' is not one of \['yearquarter', 'yearquartermonth', 'yearmonth', 'yearmonthdate', 'yearmonthdatehours', 'yearmonthdatehoursminutes', 'yearmonthdatehoursminutesseconds', 'yearweek', 'yearweekday', 'yearweekdayhours', 'yearweekdayhoursminutes', 'yearweekdayhoursminutesseconds', 'yeardayofyear', 'quartermonth', 'monthdate', 'monthdatehours', 'monthdatehoursminutes', 'monthdatehoursminutesseconds', 'weekday', 'weeksdayhours', 'weekdayhoursminutes', 'weekdayhoursminutesseconds', 'dayhours', 'dayhoursminutes', 'dayhoursminutesseconds', 'hoursminutes', 'hoursminutesseconds', 'minutesseconds', 'secondsmilliseconds'\] From aca989d1c4cf4763ee22154841363371a30ec823 Mon Sep 17 00:00:00 2001 From: Stefan Binder Date: Fri, 7 Apr 2023 12:34:29 +0000 Subject: [PATCH 05/37] Make error attribute private --- altair/utils/schemapi.py | 6 +++--- tools/schemapi/schemapi.py | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/altair/utils/schemapi.py b/altair/utils/schemapi.py index d3ce68644..c357919d5 100644 --- a/altair/utils/schemapi.py +++ b/altair/utils/schemapi.py @@ -292,7 +292,7 @@ class SchemaValidationError(jsonschema.ValidationError): def __init__(self, obj, err): super(SchemaValidationError, self).__init__(**err._contents()) self.obj = obj - self.errors = getattr(err, "_all_errors", {err.json_path: [err]}) + self._errors = getattr(err, "_all_errors", {err.json_path: [err]}) @staticmethod def _format_params_as_table(param_dict_keys): @@ -400,9 +400,9 @@ def __str__(self): additional_errors = [ err - for errors in self.errors.values() + for errors in self._errors.values() for err in errors - if err is not list(self.errors.values())[0][0] + if err is not list(self._errors.values())[0][0] ] if additional_errors: # Deduplicate error messages and only include them if they are diff --git a/tools/schemapi/schemapi.py b/tools/schemapi/schemapi.py index 0496ea7f8..7797557c9 100644 --- a/tools/schemapi/schemapi.py +++ b/tools/schemapi/schemapi.py @@ -290,7 +290,7 @@ class SchemaValidationError(jsonschema.ValidationError): def __init__(self, obj, err): super(SchemaValidationError, self).__init__(**err._contents()) self.obj = obj - self.errors = getattr(err, "_all_errors", {err.json_path: [err]}) + self._errors = getattr(err, "_all_errors", {err.json_path: [err]}) @staticmethod def _format_params_as_table(param_dict_keys): @@ -398,9 +398,9 @@ def __str__(self): additional_errors = [ err - for errors in self.errors.values() + for errors in self._errors.values() for err in errors - if err is not list(self.errors.values())[0][0] + if err is not list(self._errors.values())[0][0] ] if additional_errors: # Deduplicate error messages and only include them if they are From 3af46797727132ea8cde93a50316c3c02a1920b8 Mon Sep 17 00:00:00 2001 From: Stefan Binder Date: Fri, 7 Apr 2023 13:28:05 +0000 Subject: [PATCH 06/37] Add tests for datum and value --- tests/utils/test_schemapi.py | 42 ++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/tests/utils/test_schemapi.py b/tests/utils/test_schemapi.py index feb6f430b..d34c6ef4e 100644 --- a/tests/utils/test_schemapi.py +++ b/tests/utils/test_schemapi.py @@ -508,6 +508,24 @@ def chart_error_invalid_type(): return alt.Chart().encode(alt.X(type="unknown")) +def chart_error_additional_datum_argument(): + # Error: wrong_argument is not a valid argument to datum + return alt.Chart().mark_point().encode(x=alt.datum(1, wrong_argument=1)) + + +def chart_error_invalid_value_type(): + # Error: Value cannot be an integer in this case + return ( + alt.Chart(data.cars()) + .mark_point() + .encode( + x="Acceleration:Q", + y="Horsepower:Q", + color=alt.value(1), # should be eg. alt.value('red') + ) + ) + + @pytest.mark.parametrize( "chart_func, expected_error_message", [ @@ -627,6 +645,30 @@ def chart_error_invalid_type(): 'unknown' is not one of \['quantitative', 'ordinal', 'temporal', 'nominal', 'geojson'\]$""" ), ), + ( + chart_error_additional_datum_argument, + inspect.cleandoc( + r"""`X` has no parameter named 'wrong_argument' + + Existing parameter names are: + shorthand bin scale timeUnit + aggregate field sort title + axis impute stack type + bandPosition + + See the help for `X` to read the full description of these parameters$""" # noqa: W291 + ), + ), + ( + chart_error_invalid_value_type, + inspect.cleandoc( + r"""'1' is an invalid value for `value`: + + 1 is not of type 'object' + 1 is not of type 'string' + 1 is not of type 'null'$""" + ) + ) ], ) def test_chart_validation_errors(chart_func, expected_error_message): From e538e05ef58ea94a9c4437d4ca1d174e0c6294df Mon Sep 17 00:00:00 2001 From: Stefan Binder Date: Fri, 7 Apr 2023 13:50:35 +0000 Subject: [PATCH 07/37] Add type hints to help with refactoring --- altair/utils/schemapi.py | 30 +++++++++++++++++------------- tools/schemapi/schemapi.py | 30 +++++++++++++++++------------- 2 files changed, 34 insertions(+), 26 deletions(-) diff --git a/altair/utils/schemapi.py b/altair/utils/schemapi.py index c357919d5..22b9b694b 100644 --- a/altair/utils/schemapi.py +++ b/altair/utils/schemapi.py @@ -5,7 +5,7 @@ import inspect import json import textwrap -from typing import Any, Sequence, List, Dict, Optional, DefaultDict +from typing import Any, Sequence, List, Dict, Optional, DefaultDict, Tuple, Iterable from itertools import zip_longest import jsonschema @@ -289,15 +289,19 @@ def _resolve_references(schema, root=None): class SchemaValidationError(jsonschema.ValidationError): """A wrapper for jsonschema.ValidationError with friendlier traceback""" - def __init__(self, obj, err): - super(SchemaValidationError, self).__init__(**err._contents()) + def __init__(self, obj: "SchemaBase", err: jsonschema.ValidationError) -> None: + super().__init__(**err._contents()) self.obj = obj - self._errors = getattr(err, "_all_errors", {err.json_path: [err]}) + self._errors: GroupedValidationErrors = getattr( + err, "_all_errors", {err.json_path: [err]} + ) @staticmethod - def _format_params_as_table(param_dict_keys): + def _format_params_as_table(param_dict_keys: Iterable[str]) -> str: """Format param names into a table so that they are easier to read""" - param_names, name_lengths = zip( + param_names: Tuple[str, ...] + name_lengths: Tuple[int, ...] + param_names, name_lengths = zip( # type: ignore[assignment] # Mypy does think it's Tuple[Any] *[ (name, len(name)) for name in param_dict_keys @@ -314,15 +318,15 @@ def _format_params_as_table(param_dict_keys): columns = min(max_column_width // max_name_length, square_columns) # Compute roughly equal column heights to evenly divide the param names - def split_into_equal_parts(n, p): + def split_into_equal_parts(n: int, p: int) -> List[int]: return [n // p + 1] * (n % p) + [n // p] * (p - n % p) column_heights = split_into_equal_parts(num_param_names, columns) # Section the param names into columns and compute their widths - param_names_columns = [] - column_max_widths = [] - last_end_idx = 0 + param_names_columns: List[Tuple[str, ...]] = [] + column_max_widths: List[int] = [] + last_end_idx: int = 0 for ch in column_heights: param_names_columns.append(param_names[last_end_idx : last_end_idx + ch]) column_max_widths.append( @@ -331,11 +335,11 @@ def split_into_equal_parts(n, p): last_end_idx = ch + last_end_idx # Transpose the param name columns into rows to facilitate looping - param_names_rows = [] + param_names_rows: List[Tuple[str, ...]] = [] for li in zip_longest(*param_names_columns, fillvalue=""): param_names_rows.append(li) # Build the table as a string by iterating over and formatting the rows - param_names_table = "" + param_names_table: str = "" for param_names_row in param_names_rows: for num, param_name in enumerate(param_names_row): # Set column width based on the longest param in the column @@ -351,7 +355,7 @@ def split_into_equal_parts(n, p): param_names_table += " " * 16 return param_names_table - def __str__(self): + def __str__(self) -> str: # Try to get the lowest class possible in the chart hierarchy so # it can be displayed in the error message. This should lead to more informative # error messages pointing the user closer to the source of the issue. diff --git a/tools/schemapi/schemapi.py b/tools/schemapi/schemapi.py index 7797557c9..f5a294aca 100644 --- a/tools/schemapi/schemapi.py +++ b/tools/schemapi/schemapi.py @@ -3,7 +3,7 @@ import inspect import json import textwrap -from typing import Any, Sequence, List, Dict, Optional, DefaultDict +from typing import Any, Sequence, List, Dict, Optional, DefaultDict, Tuple, Iterable from itertools import zip_longest import jsonschema @@ -287,15 +287,19 @@ def _resolve_references(schema, root=None): class SchemaValidationError(jsonschema.ValidationError): """A wrapper for jsonschema.ValidationError with friendlier traceback""" - def __init__(self, obj, err): - super(SchemaValidationError, self).__init__(**err._contents()) + def __init__(self, obj: "SchemaBase", err: jsonschema.ValidationError) -> None: + super().__init__(**err._contents()) self.obj = obj - self._errors = getattr(err, "_all_errors", {err.json_path: [err]}) + self._errors: GroupedValidationErrors = getattr( + err, "_all_errors", {err.json_path: [err]} + ) @staticmethod - def _format_params_as_table(param_dict_keys): + def _format_params_as_table(param_dict_keys: Iterable[str]) -> str: """Format param names into a table so that they are easier to read""" - param_names, name_lengths = zip( + param_names: Tuple[str, ...] + name_lengths: Tuple[int, ...] + param_names, name_lengths = zip( # type: ignore[assignment] # Mypy does think it's Tuple[Any] *[ (name, len(name)) for name in param_dict_keys @@ -312,15 +316,15 @@ def _format_params_as_table(param_dict_keys): columns = min(max_column_width // max_name_length, square_columns) # Compute roughly equal column heights to evenly divide the param names - def split_into_equal_parts(n, p): + def split_into_equal_parts(n: int, p: int) -> List[int]: return [n // p + 1] * (n % p) + [n // p] * (p - n % p) column_heights = split_into_equal_parts(num_param_names, columns) # Section the param names into columns and compute their widths - param_names_columns = [] - column_max_widths = [] - last_end_idx = 0 + param_names_columns: List[Tuple[str, ...]] = [] + column_max_widths: List[int] = [] + last_end_idx: int = 0 for ch in column_heights: param_names_columns.append(param_names[last_end_idx : last_end_idx + ch]) column_max_widths.append( @@ -329,11 +333,11 @@ def split_into_equal_parts(n, p): last_end_idx = ch + last_end_idx # Transpose the param name columns into rows to facilitate looping - param_names_rows = [] + param_names_rows: List[Tuple[str, ...]] = [] for li in zip_longest(*param_names_columns, fillvalue=""): param_names_rows.append(li) # Build the table as a string by iterating over and formatting the rows - param_names_table = "" + param_names_table: str = "" for param_names_row in param_names_rows: for num, param_name in enumerate(param_names_row): # Set column width based on the longest param in the column @@ -349,7 +353,7 @@ def split_into_equal_parts(n, p): param_names_table += " " * 16 return param_names_table - def __str__(self): + def __str__(self) -> str: # Try to get the lowest class possible in the chart hierarchy so # it can be displayed in the error message. This should lead to more informative # error messages pointing the user closer to the source of the issue. From 7d8c9391b387b82d27d870a9c175d30b18e53997 Mon Sep 17 00:00:00 2001 From: Stefan Binder Date: Fri, 7 Apr 2023 16:20:23 +0000 Subject: [PATCH 08/37] Refactor creation of error message as preparation for handling multiple errors --- altair/utils/schemapi.py | 60 ++++++++++++++++++++++++++------------ tools/schemapi/schemapi.py | 60 ++++++++++++++++++++++++++------------ 2 files changed, 82 insertions(+), 38 deletions(-) diff --git a/altair/utils/schemapi.py b/altair/utils/schemapi.py index 22b9b694b..728dc1261 100644 --- a/altair/utils/schemapi.py +++ b/altair/utils/schemapi.py @@ -5,7 +5,17 @@ import inspect import json import textwrap -from typing import Any, Sequence, List, Dict, Optional, DefaultDict, Tuple, Iterable +from typing import ( + Any, + Sequence, + List, + Dict, + Optional, + DefaultDict, + Tuple, + Iterable, + Type, +) from itertools import zip_longest import jsonschema @@ -356,25 +366,17 @@ def split_into_equal_parts(n: int, p: int) -> List[int]: return param_names_table def __str__(self) -> str: - # Try to get the lowest class possible in the chart hierarchy so - # it can be displayed in the error message. This should lead to more informative - # error messages pointing the user closer to the source of the issue. - for prop_name in reversed(self.absolute_path): - # Check if str as e.g. first item can be a 0 - if isinstance(prop_name, str): - potential_class_name = prop_name[0].upper() + prop_name[1:] - cls = getattr(vegalite, potential_class_name, None) - if cls is not None: - break - else: - # Did not find a suitable class based on traversing the path so we fall - # back on the class of the top-level object which created - # the SchemaValidationError - cls = self.obj.__class__ + altair_cls = self._get_altair_class_for_error(self) + return self._get_message_for_error(self, altair_cls) + def _get_message_for_error( + self, + error: jsonschema.exceptions.ValidationError, + altair_cls: Type["SchemaBase"], + ) -> str: # Output all existing parameters when an unknown parameter is specified if self.validator == "additionalProperties": - param_dict_keys = inspect.signature(cls).parameters.keys() + param_dict_keys = inspect.signature(altair_cls).parameters.keys() param_names_table = self._format_params_as_table(param_dict_keys) # `cleandoc` removes multiline string indentation in the output @@ -385,10 +387,10 @@ def __str__(self) -> str: {} See the help for `{}` to read the full description of these parameters """.format( - cls.__name__, + altair_cls.__name__, self.message.split("('")[-1].split("'")[0], param_names_table, - cls.__name__, + altair_cls.__name__, ) ) # Use the default error message for all other cases than unknown parameter errors @@ -434,6 +436,26 @@ def __str__(self) -> str: ) ) + def _get_altair_class_for_error( + self, error: jsonschema.exceptions.ValidationError + ) -> Type["SchemaBase"]: + # Try to get the lowest class possible in the chart hierarchy so + # it can be displayed in the error message. This should lead to more informative + # error messages pointing the user closer to the source of the issue. + for prop_name in reversed(error.absolute_path): + # Check if str as e.g. first item can be a 0 + if isinstance(prop_name, str): + potential_class_name = prop_name[0].upper() + prop_name[1:] + cls = getattr(vegalite, potential_class_name, None) + if cls is not None: + break + else: + # Did not find a suitable class based on traversing the path so we fall + # back on the class of the top-level object which created + # the SchemaValidationError + cls = self.obj.__class__ + return cls + class UndefinedType: """A singleton object for marking undefined parameters""" diff --git a/tools/schemapi/schemapi.py b/tools/schemapi/schemapi.py index f5a294aca..72ae89474 100644 --- a/tools/schemapi/schemapi.py +++ b/tools/schemapi/schemapi.py @@ -3,7 +3,17 @@ import inspect import json import textwrap -from typing import Any, Sequence, List, Dict, Optional, DefaultDict, Tuple, Iterable +from typing import ( + Any, + Sequence, + List, + Dict, + Optional, + DefaultDict, + Tuple, + Iterable, + Type, +) from itertools import zip_longest import jsonschema @@ -354,25 +364,17 @@ def split_into_equal_parts(n: int, p: int) -> List[int]: return param_names_table def __str__(self) -> str: - # Try to get the lowest class possible in the chart hierarchy so - # it can be displayed in the error message. This should lead to more informative - # error messages pointing the user closer to the source of the issue. - for prop_name in reversed(self.absolute_path): - # Check if str as e.g. first item can be a 0 - if isinstance(prop_name, str): - potential_class_name = prop_name[0].upper() + prop_name[1:] - cls = getattr(vegalite, potential_class_name, None) - if cls is not None: - break - else: - # Did not find a suitable class based on traversing the path so we fall - # back on the class of the top-level object which created - # the SchemaValidationError - cls = self.obj.__class__ + altair_cls = self._get_altair_class_for_error(self) + return self._get_message_for_error(self, altair_cls) + def _get_message_for_error( + self, + error: jsonschema.exceptions.ValidationError, + altair_cls: Type["SchemaBase"], + ) -> str: # Output all existing parameters when an unknown parameter is specified if self.validator == "additionalProperties": - param_dict_keys = inspect.signature(cls).parameters.keys() + param_dict_keys = inspect.signature(altair_cls).parameters.keys() param_names_table = self._format_params_as_table(param_dict_keys) # `cleandoc` removes multiline string indentation in the output @@ -383,10 +385,10 @@ def __str__(self) -> str: {} See the help for `{}` to read the full description of these parameters """.format( - cls.__name__, + altair_cls.__name__, self.message.split("('")[-1].split("'")[0], param_names_table, - cls.__name__, + altair_cls.__name__, ) ) # Use the default error message for all other cases than unknown parameter errors @@ -432,6 +434,26 @@ def __str__(self) -> str: ) ) + def _get_altair_class_for_error( + self, error: jsonschema.exceptions.ValidationError + ) -> Type["SchemaBase"]: + # Try to get the lowest class possible in the chart hierarchy so + # it can be displayed in the error message. This should lead to more informative + # error messages pointing the user closer to the source of the issue. + for prop_name in reversed(error.absolute_path): + # Check if str as e.g. first item can be a 0 + if isinstance(prop_name, str): + potential_class_name = prop_name[0].upper() + prop_name[1:] + cls = getattr(vegalite, potential_class_name, None) + if cls is not None: + break + else: + # Did not find a suitable class based on traversing the path so we fall + # back on the class of the top-level object which created + # the SchemaValidationError + cls = self.obj.__class__ + return cls + class UndefinedType: """A singleton object for marking undefined parameters""" From a45d232bec8fc523a9ceb680c4a58a17a20215c4 Mon Sep 17 00:00:00 2001 From: Stefan Binder Date: Fri, 7 Apr 2023 16:49:42 +0000 Subject: [PATCH 09/37] First draft for showing all distinct errors --- altair/utils/schemapi.py | 32 ++++++++--------- tests/utils/test_schemapi.py | 69 +++++++++++++++++++++++++++++++++--- tools/schemapi/schemapi.py | 32 ++++++++--------- 3 files changed, 97 insertions(+), 36 deletions(-) diff --git a/altair/utils/schemapi.py b/altair/utils/schemapi.py index 728dc1261..ac83b4e9d 100644 --- a/altair/utils/schemapi.py +++ b/altair/utils/schemapi.py @@ -366,16 +366,20 @@ def split_into_equal_parts(n: int, p: int) -> List[int]: return param_names_table def __str__(self) -> str: - altair_cls = self._get_altair_class_for_error(self) - return self._get_message_for_error(self, altair_cls) + messages: List[str] = [] + for _, errors in self._errors.items(): + altair_cls = self._get_altair_class_for_error(errors[0]) + messages.append(self._get_message_for_errors(errors, altair_cls)) + return ("\n" + "-" * 50 + "\n").join(messages) - def _get_message_for_error( + def _get_message_for_errors( self, - error: jsonschema.exceptions.ValidationError, + errors: Sequence[jsonschema.exceptions.ValidationError], altair_cls: Type["SchemaBase"], ) -> str: + error = errors[0] # Output all existing parameters when an unknown parameter is specified - if self.validator == "additionalProperties": + if error.validator == "additionalProperties": param_dict_keys = inspect.signature(altair_cls).parameters.keys() param_names_table = self._format_params_as_table(param_dict_keys) @@ -388,28 +392,24 @@ def _get_message_for_error( See the help for `{}` to read the full description of these parameters """.format( altair_cls.__name__, - self.message.split("('")[-1].split("'")[0], + error.message.split("('")[-1].split("'")[0], param_names_table, altair_cls.__name__, ) ) - # Use the default error message for all other cases than unknown parameter errors + # Use the default error message for all other cases than unknown + # parameter errors else: - message = self.message + message = error.message # Add a summary line when parameters are passed an invalid value # For example: "'asdf' is an invalid value for `stack` - if self.absolute_path: + if error.absolute_path: # The indentation here must match that of `cleandoc` below - message = f"""'{self.instance}' is an invalid value for `{self.absolute_path[-1]}`: + message = f"""'{error.instance}' is an invalid value for `{error.absolute_path[-1]}`: {message}""" - additional_errors = [ - err - for errors in self._errors.values() - for err in errors - if err is not list(self._errors.values())[0][0] - ] + additional_errors = [err for err in errors if err is not error] if additional_errors: # Deduplicate error messages and only include them if they are # different then the main error message stored in self.message. diff --git a/tests/utils/test_schemapi.py b/tests/utils/test_schemapi.py index d34c6ef4e..5c516a428 100644 --- a/tests/utils/test_schemapi.py +++ b/tests/utils/test_schemapi.py @@ -446,7 +446,8 @@ def chart_error_example_invalid_channel(): def chart_error_example_invalid_y_option_value_unknown_x_option(): - # Error: Invalid Y option value "asdf" and unknown option "unknown" for X + # Error 1: unknown is an invalid channel option for X + # Error 2: Invalid Y option value "asdf" and unknown option "unknown" for X return ( alt.Chart(data.barley()) .mark_bar() @@ -526,6 +527,25 @@ def chart_error_invalid_value_type(): ) +def chart_error_two_errors_in_layered_chart(): + # Error 1: Wrong data type to pass to tooltip + # Error 2: invalidChannel is not a valid encoding channel + return alt.layer( + alt.Chart().mark_point().encode(tooltip=[{"wrong"}]), + alt.Chart().mark_line().encode(invalidChannel="unknown"), + ) + + +def chart_error_two_errors_in_complex_concat_layered_chart(): + # Error 1: Wrong data type to pass to tooltip + # Error 2: Invalid value for bandPosition + return alt.layer(alt.Chart().mark_point().encode(tooltip=[{"wrong"}])) | ( + alt.Chart(data.cars()) + .mark_text(align="right") + .encode(alt.Text("Horsepower:N", bandPosition="4")) + ) + + @pytest.mark.parametrize( "chart_func, expected_error_message", [ @@ -540,9 +560,50 @@ def chart_error_invalid_value_type(): axis impute stack type bandPosition - See the help for `X` to read the full description of these parameters$""" # noqa: W291 + See the help for `X` to read the full description of these parameters + -------------------------------------------------- + 'asdf' is an invalid value for `stack`: + + 'asdf' is not one of \['zero', 'center', 'normalize'\] + 'asdf' is not of type 'null' + 'asdf' is not of type 'boolean'$""" # noqa: W291 + ), + ), + ( + chart_error_two_errors_in_layered_chart, + inspect.cleandoc( + r"""'{'wrong'}' is an invalid value for `field`: + + {'wrong'} is not of type 'string' + {'wrong'} is not of type 'object' + -------------------------------------------------- + `Encoding` has no parameter named 'invalidChannel' + + Existing parameter names are: + angle key order strokeDash tooltip xOffset + color latitude radius strokeOpacity url y + description latitude2 radius2 strokeWidth x y2 + detail longitude shape text x2 yError + fill longitude2 size theta xError yError2 + fillOpacity opacity stroke theta2 xError2 yOffset + href + + See the help for `Encoding` to read the full description of these parameters$""" # noqa: W291 ), ), + ( + chart_error_two_errors_in_complex_concat_layered_chart, + inspect.cleandoc( + r"""'{'wrong'}' is an invalid value for `field`: + + {'wrong'} is not of type 'string' + {'wrong'} is not of type 'object' + -------------------------------------------------- + '4' is an invalid value for `bandPosition`: + + '4' is not of type 'number'$""" + ) + ), ( chart_error_example_layer, inspect.cleandoc( @@ -667,8 +728,8 @@ def chart_error_invalid_value_type(): 1 is not of type 'object' 1 is not of type 'string' 1 is not of type 'null'$""" - ) - ) + ), + ), ], ) def test_chart_validation_errors(chart_func, expected_error_message): diff --git a/tools/schemapi/schemapi.py b/tools/schemapi/schemapi.py index 72ae89474..29e3d443f 100644 --- a/tools/schemapi/schemapi.py +++ b/tools/schemapi/schemapi.py @@ -364,16 +364,20 @@ def split_into_equal_parts(n: int, p: int) -> List[int]: return param_names_table def __str__(self) -> str: - altair_cls = self._get_altair_class_for_error(self) - return self._get_message_for_error(self, altair_cls) + messages: List[str] = [] + for _, errors in self._errors.items(): + altair_cls = self._get_altair_class_for_error(errors[0]) + messages.append(self._get_message_for_errors(errors, altair_cls)) + return ("\n" + "-" * 50 + "\n").join(messages) - def _get_message_for_error( + def _get_message_for_errors( self, - error: jsonschema.exceptions.ValidationError, + errors: Sequence[jsonschema.exceptions.ValidationError], altair_cls: Type["SchemaBase"], ) -> str: + error = errors[0] # Output all existing parameters when an unknown parameter is specified - if self.validator == "additionalProperties": + if error.validator == "additionalProperties": param_dict_keys = inspect.signature(altair_cls).parameters.keys() param_names_table = self._format_params_as_table(param_dict_keys) @@ -386,28 +390,24 @@ def _get_message_for_error( See the help for `{}` to read the full description of these parameters """.format( altair_cls.__name__, - self.message.split("('")[-1].split("'")[0], + error.message.split("('")[-1].split("'")[0], param_names_table, altair_cls.__name__, ) ) - # Use the default error message for all other cases than unknown parameter errors + # Use the default error message for all other cases than unknown + # parameter errors else: - message = self.message + message = error.message # Add a summary line when parameters are passed an invalid value # For example: "'asdf' is an invalid value for `stack` - if self.absolute_path: + if error.absolute_path: # The indentation here must match that of `cleandoc` below - message = f"""'{self.instance}' is an invalid value for `{self.absolute_path[-1]}`: + message = f"""'{error.instance}' is an invalid value for `{error.absolute_path[-1]}`: {message}""" - additional_errors = [ - err - for errors in self._errors.values() - for err in errors - if err is not list(self._errors.values())[0][0] - ] + additional_errors = [err for err in errors if err is not error] if additional_errors: # Deduplicate error messages and only include them if they are # different then the main error message stored in self.message. From 2cfb9cdd3d35f8268907b80ff202429492c9d845 Mon Sep 17 00:00:00 2001 From: Stefan Binder Date: Sat, 8 Apr 2023 14:10:38 +0000 Subject: [PATCH 10/37] Type hint validate_jsonschema --- altair/utils/schemapi.py | 21 +++++++++++++++++---- tools/schemapi/schemapi.py | 21 +++++++++++++++++---- 2 files changed, 34 insertions(+), 8 deletions(-) diff --git a/altair/utils/schemapi.py b/altair/utils/schemapi.py index ac83b4e9d..3b4c169b0 100644 --- a/altair/utils/schemapi.py +++ b/altair/utils/schemapi.py @@ -59,7 +59,12 @@ def debug_mode(arg): DEBUG_MODE = original -def validate_jsonschema(spec, schema, rootschema=None, raise_error=True): +def validate_jsonschema( + spec: Dict[str, Any], + schema: Dict[str, Any], + rootschema: Optional[Dict[str, Any]] = None, + raise_error: bool = True, +) -> Optional[jsonschema.exceptions.ValidationError]: errors = _get_errors_from_spec(spec, schema, rootschema=rootschema) if errors: leaf_errors = _get_leaves_of_error_tree(errors) @@ -72,14 +77,20 @@ def validate_jsonschema(spec, schema, rootschema=None, raise_error=True): main_error = list(grouped_errors.values())[0][0] # They can be used to craft more helpful error messages when this error # is being converted to a SchemaValidationError - main_error._all_errors = grouped_errors + main_error._all_errors = grouped_errors # type: ignore[attr-defined] if raise_error: raise main_error else: return main_error + else: + return None -def _get_errors_from_spec(spec, schema, rootschema=None) -> ValidationErrorList: +def _get_errors_from_spec( + spec: Dict[str, Any], + schema: Dict[str, Any], + rootschema: Optional[Dict[str, Any]] = None, +) -> ValidationErrorList: # We don't use jsonschema.validate as this would validate the schema itself. # Instead, we pass the schema directly to the validator class. This is done for # two reasons: The schema comes from Vega-Lite and is not based on the user @@ -126,7 +137,9 @@ def _get_leaves_of_error_tree( return leaves -def _subset_to_most_specific_json_paths(errors_by_json_path: GroupedValidationErrors): +def _subset_to_most_specific_json_paths( + errors_by_json_path: GroupedValidationErrors, +) -> GroupedValidationErrors: errors_by_json_path_specific: GroupedValidationErrors = {} for json_path, errors in errors_by_json_path.items(): if not _contained_at_start_of_one_of_other_values( diff --git a/tools/schemapi/schemapi.py b/tools/schemapi/schemapi.py index 29e3d443f..9a376f687 100644 --- a/tools/schemapi/schemapi.py +++ b/tools/schemapi/schemapi.py @@ -57,7 +57,12 @@ def debug_mode(arg): DEBUG_MODE = original -def validate_jsonschema(spec, schema, rootschema=None, raise_error=True): +def validate_jsonschema( + spec: Dict[str, Any], + schema: Dict[str, Any], + rootschema: Optional[Dict[str, Any]] = None, + raise_error: bool = True, +) -> Optional[jsonschema.exceptions.ValidationError]: errors = _get_errors_from_spec(spec, schema, rootschema=rootschema) if errors: leaf_errors = _get_leaves_of_error_tree(errors) @@ -70,14 +75,20 @@ def validate_jsonschema(spec, schema, rootschema=None, raise_error=True): main_error = list(grouped_errors.values())[0][0] # They can be used to craft more helpful error messages when this error # is being converted to a SchemaValidationError - main_error._all_errors = grouped_errors + main_error._all_errors = grouped_errors # type: ignore[attr-defined] if raise_error: raise main_error else: return main_error + else: + return None -def _get_errors_from_spec(spec, schema, rootschema=None) -> ValidationErrorList: +def _get_errors_from_spec( + spec: Dict[str, Any], + schema: Dict[str, Any], + rootschema: Optional[Dict[str, Any]] = None, +) -> ValidationErrorList: # We don't use jsonschema.validate as this would validate the schema itself. # Instead, we pass the schema directly to the validator class. This is done for # two reasons: The schema comes from Vega-Lite and is not based on the user @@ -124,7 +135,9 @@ def _get_leaves_of_error_tree( return leaves -def _subset_to_most_specific_json_paths(errors_by_json_path: GroupedValidationErrors): +def _subset_to_most_specific_json_paths( + errors_by_json_path: GroupedValidationErrors, +) -> GroupedValidationErrors: errors_by_json_path_specific: GroupedValidationErrors = {} for json_path, errors in errors_by_json_path.items(): if not _contained_at_start_of_one_of_other_values( From ef10d230b0519050f1b928be530143325ff9a74e Mon Sep 17 00:00:00 2001 From: Stefan Binder Date: Sat, 8 Apr 2023 14:15:12 +0000 Subject: [PATCH 11/37] Add correct error message to attribute of SchemaValidationError --- altair/utils/schemapi.py | 21 ++++++++++++++------- tools/schemapi/schemapi.py | 21 ++++++++++++++------- 2 files changed, 28 insertions(+), 14 deletions(-) diff --git a/altair/utils/schemapi.py b/altair/utils/schemapi.py index 3b4c169b0..4e14a21f7 100644 --- a/altair/utils/schemapi.py +++ b/altair/utils/schemapi.py @@ -318,6 +318,20 @@ def __init__(self, obj: "SchemaBase", err: jsonschema.ValidationError) -> None: self._errors: GroupedValidationErrors = getattr( err, "_all_errors", {err.json_path: [err]} ) + # This is the message from err + self._original_message = self.message + self.message = self._get_message() + + def __str__(self) -> str: + return self.message + + def _get_message(self) -> str: + messages: List[str] = [] + for _, errors in self._errors.items(): + altair_cls = self._get_altair_class_for_error(errors[0]) + messages.append(self._get_message_for_errors(errors, altair_cls)) + return ("\n" + "-" * 50 + "\n").join(messages) + @staticmethod def _format_params_as_table(param_dict_keys: Iterable[str]) -> str: @@ -378,13 +392,6 @@ def split_into_equal_parts(n: int, p: int) -> List[int]: param_names_table += " " * 16 return param_names_table - def __str__(self) -> str: - messages: List[str] = [] - for _, errors in self._errors.items(): - altair_cls = self._get_altair_class_for_error(errors[0]) - messages.append(self._get_message_for_errors(errors, altair_cls)) - return ("\n" + "-" * 50 + "\n").join(messages) - def _get_message_for_errors( self, errors: Sequence[jsonschema.exceptions.ValidationError], diff --git a/tools/schemapi/schemapi.py b/tools/schemapi/schemapi.py index 9a376f687..918e6f99f 100644 --- a/tools/schemapi/schemapi.py +++ b/tools/schemapi/schemapi.py @@ -316,6 +316,20 @@ def __init__(self, obj: "SchemaBase", err: jsonschema.ValidationError) -> None: self._errors: GroupedValidationErrors = getattr( err, "_all_errors", {err.json_path: [err]} ) + # This is the message from err + self._original_message = self.message + self.message = self._get_message() + + def __str__(self) -> str: + return self.message + + def _get_message(self) -> str: + messages: List[str] = [] + for _, errors in self._errors.items(): + altair_cls = self._get_altair_class_for_error(errors[0]) + messages.append(self._get_message_for_errors(errors, altair_cls)) + return ("\n" + "-" * 50 + "\n").join(messages) + @staticmethod def _format_params_as_table(param_dict_keys: Iterable[str]) -> str: @@ -376,13 +390,6 @@ def split_into_equal_parts(n: int, p: int) -> List[int]: param_names_table += " " * 16 return param_names_table - def __str__(self) -> str: - messages: List[str] = [] - for _, errors in self._errors.items(): - altair_cls = self._get_altair_class_for_error(errors[0]) - messages.append(self._get_message_for_errors(errors, altair_cls)) - return ("\n" + "-" * 50 + "\n").join(messages) - def _get_message_for_errors( self, errors: Sequence[jsonschema.exceptions.ValidationError], From 12857eb8367c0c434362ceffc389bec47ec4e3dd Mon Sep 17 00:00:00 2001 From: Stefan Binder Date: Sat, 8 Apr 2023 14:18:41 +0000 Subject: [PATCH 12/37] Remove unnecessary deduplication in schemavalidationerror which now already happens in validate_jsonschema --- altair/utils/schemapi.py | 18 ++---------------- tools/schemapi/schemapi.py | 18 ++---------------- 2 files changed, 4 insertions(+), 32 deletions(-) diff --git a/altair/utils/schemapi.py b/altair/utils/schemapi.py index 4e14a21f7..e98b41eae 100644 --- a/altair/utils/schemapi.py +++ b/altair/utils/schemapi.py @@ -431,23 +431,9 @@ def _get_message_for_errors( additional_errors = [err for err in errors if err is not error] if additional_errors: - # Deduplicate error messages and only include them if they are - # different then the main error message stored in self.message. - # Using dict instead of set to keep the original order in case it was - # chosen intentionally to move more relevant error messages to the top - additional_error_messages = list( - dict.fromkeys( - [ - e.message - for e in additional_errors - if e.message != self.message - ] - ) + message += "\n " + "\n ".join( + [e.message for e in additional_errors] ) - if additional_error_messages: - message += "\n " + "\n ".join( - additional_error_messages - ) return inspect.cleandoc( """{} diff --git a/tools/schemapi/schemapi.py b/tools/schemapi/schemapi.py index 918e6f99f..8a168c99b 100644 --- a/tools/schemapi/schemapi.py +++ b/tools/schemapi/schemapi.py @@ -429,23 +429,9 @@ def _get_message_for_errors( additional_errors = [err for err in errors if err is not error] if additional_errors: - # Deduplicate error messages and only include them if they are - # different then the main error message stored in self.message. - # Using dict instead of set to keep the original order in case it was - # chosen intentionally to move more relevant error messages to the top - additional_error_messages = list( - dict.fromkeys( - [ - e.message - for e in additional_errors - if e.message != self.message - ] - ) + message += "\n " + "\n ".join( + [e.message for e in additional_errors] ) - if additional_error_messages: - message += "\n " + "\n ".join( - additional_error_messages - ) return inspect.cleandoc( """{} From e6c4df915d147536a90cc9c1c16e5185ef411931 Mon Sep 17 00:00:00 2001 From: Stefan Binder Date: Sat, 8 Apr 2023 14:25:40 +0000 Subject: [PATCH 13/37] Remove _get_all_lowest_errors_with_validator --- altair/utils/schemapi.py | 13 ------------- tools/schemapi/schemapi.py | 13 ------------- 2 files changed, 26 deletions(-) diff --git a/altair/utils/schemapi.py b/altair/utils/schemapi.py index e98b41eae..7a08f6692 100644 --- a/altair/utils/schemapi.py +++ b/altair/utils/schemapi.py @@ -258,19 +258,6 @@ def _deduplicate_by_message(errors: ValidationErrorList) -> ValidationErrorList: return list({e.message: e for e in errors}.values()) -def _get_all_lowest_errors_with_validator( - error: jsonschema.ValidationError, validator: str -) -> List[jsonschema.ValidationError]: - matches: List[jsonschema.ValidationError] = [] - if error.context: - for err in error.context: - if err.context: - matches.extend(_get_all_lowest_errors_with_validator(err, validator)) - elif err.validator == validator: - matches.append(err) - return matches - - def _subclasses(cls): """Breadth-first sequence of all classes which inherit from cls.""" seen = set() diff --git a/tools/schemapi/schemapi.py b/tools/schemapi/schemapi.py index 8a168c99b..7e8642337 100644 --- a/tools/schemapi/schemapi.py +++ b/tools/schemapi/schemapi.py @@ -256,19 +256,6 @@ def _deduplicate_by_message(errors: ValidationErrorList) -> ValidationErrorList: return list({e.message: e for e in errors}.values()) -def _get_all_lowest_errors_with_validator( - error: jsonschema.ValidationError, validator: str -) -> List[jsonschema.ValidationError]: - matches: List[jsonschema.ValidationError] = [] - if error.context: - for err in error.context: - if err.context: - matches.extend(_get_all_lowest_errors_with_validator(err, validator)) - elif err.validator == validator: - matches.append(err) - return matches - - def _subclasses(cls): """Breadth-first sequence of all classes which inherit from cls.""" seen = set() From 8848bb3c418170ccde7cb1dd25a2de31405c6525 Mon Sep 17 00:00:00 2001 From: Stefan Binder Date: Sat, 8 Apr 2023 14:28:22 +0000 Subject: [PATCH 14/37] Add PR to changelog. Only show one line for error message improvements in changelog --- doc/releases/changes.rst | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/doc/releases/changes.rst b/doc/releases/changes.rst index c571d395e..ade075aed 100644 --- a/doc/releases/changes.rst +++ b/doc/releases/changes.rst @@ -21,10 +21,9 @@ Enhancements - Saving charts with HTML inline is now supported without having altair_saver installed (#2807). - The documentation page has been revamped, both in terms of appearance and content. - More informative autocompletion by removing deprecated methods (#2814) and for editors that rely on type hints (e.g. VS Code) we added support for completion in method chains (#2846) and extended keyword completion to cover additional methods (#2920). -- Substantially improved error handling. Both in terms of finding the more relevant error (#2842), and in terms of improving the formatting and clarity of the error messages (#2824, #2568). +- Substantially improved error handling. Both in terms of finding the more relevant error (#2842), and in terms of improving the formatting and clarity of the error messages (#2824, #2568, #2979, #3009). - Include experimental support for the DataFrame Interchange Protocol (through ``__dataframe__`` attribute). This requires ``pyarrow>=11.0.0`` (#2888). - Support data type inference for columns with special characters (#2905). -- Changed error message to be more informative when two field strings are using in a condition (#2979). Grammar Changes ~~~~~~~~~~~~~~~ From 473294eba0be3a773de71e3b6850deeca428bc6f Mon Sep 17 00:00:00 2001 From: Stefan Binder Date: Sat, 8 Apr 2023 14:41:44 +0000 Subject: [PATCH 15/37] Remove indentation in error messages which had to be removed with inspect.cleandoc. This makes it easier to work on the error messages --- altair/utils/schemapi.py | 44 ++++++++++++++------------------------ tools/schemapi/schemapi.py | 44 ++++++++++++++------------------------ 2 files changed, 32 insertions(+), 56 deletions(-) diff --git a/altair/utils/schemapi.py b/altair/utils/schemapi.py index 7a08f6692..fd5e3404b 100644 --- a/altair/utils/schemapi.py +++ b/altair/utils/schemapi.py @@ -319,7 +319,6 @@ def _get_message(self) -> str: messages.append(self._get_message_for_errors(errors, altair_cls)) return ("\n" + "-" * 50 + "\n").join(messages) - @staticmethod def _format_params_as_table(param_dict_keys: Iterable[str]) -> str: """Format param names into a table so that they are easier to read""" @@ -375,8 +374,6 @@ def split_into_equal_parts(n: int, p: int) -> List[int]: # Insert newlines and spacing after the last element in each row if num == (len(param_names_row) - 1): param_names_table += "\n" - # 16 is the indendation of the returned multiline string below - param_names_table += " " * 16 return param_names_table def _get_message_for_errors( @@ -390,20 +387,18 @@ def _get_message_for_errors( param_dict_keys = inspect.signature(altair_cls).parameters.keys() param_names_table = self._format_params_as_table(param_dict_keys) - # `cleandoc` removes multiline string indentation in the output - return inspect.cleandoc( - """`{}` has no parameter named {!r} - - Existing parameter names are: - {} - See the help for `{}` to read the full description of these parameters - """.format( - altair_cls.__name__, - error.message.split("('")[-1].split("'")[0], - param_names_table, - altair_cls.__name__, - ) + message = """\ +`{}` has no parameter named {!r} + +Existing parameter names are: +{} +See the help for `{}` to read the full description of these parameters""".format( + altair_cls.__name__, + error.message.split("('")[-1].split("'")[0], + param_names_table, + altair_cls.__name__, ) + return message # Use the default error message for all other cases than unknown # parameter errors else: @@ -411,23 +406,16 @@ def _get_message_for_errors( # Add a summary line when parameters are passed an invalid value # For example: "'asdf' is an invalid value for `stack` if error.absolute_path: - # The indentation here must match that of `cleandoc` below - message = f"""'{error.instance}' is an invalid value for `{error.absolute_path[-1]}`: + message = f"""\ +'{error.instance}' is an invalid value for `{error.absolute_path[-1]}`: - {message}""" +{message}""" additional_errors = [err for err in errors if err is not error] if additional_errors: - message += "\n " + "\n ".join( - [e.message for e in additional_errors] - ) + message += "\n" + "\n".join([e.message for e in additional_errors]) - return inspect.cleandoc( - """{} - """.format( - message - ) - ) + return message def _get_altair_class_for_error( self, error: jsonschema.exceptions.ValidationError diff --git a/tools/schemapi/schemapi.py b/tools/schemapi/schemapi.py index 7e8642337..2ac95067d 100644 --- a/tools/schemapi/schemapi.py +++ b/tools/schemapi/schemapi.py @@ -317,7 +317,6 @@ def _get_message(self) -> str: messages.append(self._get_message_for_errors(errors, altair_cls)) return ("\n" + "-" * 50 + "\n").join(messages) - @staticmethod def _format_params_as_table(param_dict_keys: Iterable[str]) -> str: """Format param names into a table so that they are easier to read""" @@ -373,8 +372,6 @@ def split_into_equal_parts(n: int, p: int) -> List[int]: # Insert newlines and spacing after the last element in each row if num == (len(param_names_row) - 1): param_names_table += "\n" - # 16 is the indendation of the returned multiline string below - param_names_table += " " * 16 return param_names_table def _get_message_for_errors( @@ -388,20 +385,18 @@ def _get_message_for_errors( param_dict_keys = inspect.signature(altair_cls).parameters.keys() param_names_table = self._format_params_as_table(param_dict_keys) - # `cleandoc` removes multiline string indentation in the output - return inspect.cleandoc( - """`{}` has no parameter named {!r} - - Existing parameter names are: - {} - See the help for `{}` to read the full description of these parameters - """.format( - altair_cls.__name__, - error.message.split("('")[-1].split("'")[0], - param_names_table, - altair_cls.__name__, - ) + message = """\ +`{}` has no parameter named {!r} + +Existing parameter names are: +{} +See the help for `{}` to read the full description of these parameters""".format( + altair_cls.__name__, + error.message.split("('")[-1].split("'")[0], + param_names_table, + altair_cls.__name__, ) + return message # Use the default error message for all other cases than unknown # parameter errors else: @@ -409,23 +404,16 @@ def _get_message_for_errors( # Add a summary line when parameters are passed an invalid value # For example: "'asdf' is an invalid value for `stack` if error.absolute_path: - # The indentation here must match that of `cleandoc` below - message = f"""'{error.instance}' is an invalid value for `{error.absolute_path[-1]}`: + message = f"""\ +'{error.instance}' is an invalid value for `{error.absolute_path[-1]}`: - {message}""" +{message}""" additional_errors = [err for err in errors if err is not error] if additional_errors: - message += "\n " + "\n ".join( - [e.message for e in additional_errors] - ) + message += "\n" + "\n".join([e.message for e in additional_errors]) - return inspect.cleandoc( - """{} - """.format( - message - ) - ) + return message def _get_altair_class_for_error( self, error: jsonschema.exceptions.ValidationError From 6f0ddfcdc25fdbda623084e6f3881caf2ddd9023 Mon Sep 17 00:00:00 2001 From: Stefan Binder Date: Mon, 10 Apr 2023 09:03:17 +0000 Subject: [PATCH 16/37] Use f string instead of format --- altair/utils/schemapi.py | 20 +++++++++----------- tools/schemapi/schemapi.py | 20 +++++++++----------- 2 files changed, 18 insertions(+), 22 deletions(-) diff --git a/altair/utils/schemapi.py b/altair/utils/schemapi.py index fd5e3404b..3ce499515 100644 --- a/altair/utils/schemapi.py +++ b/altair/utils/schemapi.py @@ -387,18 +387,16 @@ def _get_message_for_errors( param_dict_keys = inspect.signature(altair_cls).parameters.keys() param_names_table = self._format_params_as_table(param_dict_keys) - message = """\ -`{}` has no parameter named {!r} + # Error messages for these errors look like this: + # "Additional properties are not allowed ('unknown' was unexpected)" + # Line below extracts "unknown" from this string + parameter_name = error.message.split("('")[-1].split("'")[0] + message = f"""\ +`{altair_cls.__name__}` has no parameter named '{parameter_name}' Existing parameter names are: -{} -See the help for `{}` to read the full description of these parameters""".format( - altair_cls.__name__, - error.message.split("('")[-1].split("'")[0], - param_names_table, - altair_cls.__name__, - ) - return message +{param_names_table} +See the help for `{altair_cls.__name__}` to read the full description of these parameters""" # Use the default error message for all other cases than unknown # parameter errors else: @@ -415,7 +413,7 @@ def _get_message_for_errors( if additional_errors: message += "\n" + "\n".join([e.message for e in additional_errors]) - return message + return message.strip() def _get_altair_class_for_error( self, error: jsonschema.exceptions.ValidationError diff --git a/tools/schemapi/schemapi.py b/tools/schemapi/schemapi.py index 2ac95067d..e11570b9e 100644 --- a/tools/schemapi/schemapi.py +++ b/tools/schemapi/schemapi.py @@ -385,18 +385,16 @@ def _get_message_for_errors( param_dict_keys = inspect.signature(altair_cls).parameters.keys() param_names_table = self._format_params_as_table(param_dict_keys) - message = """\ -`{}` has no parameter named {!r} + # Error messages for these errors look like this: + # "Additional properties are not allowed ('unknown' was unexpected)" + # Line below extracts "unknown" from this string + parameter_name = error.message.split("('")[-1].split("'")[0] + message = f"""\ +`{altair_cls.__name__}` has no parameter named '{parameter_name}' Existing parameter names are: -{} -See the help for `{}` to read the full description of these parameters""".format( - altair_cls.__name__, - error.message.split("('")[-1].split("'")[0], - param_names_table, - altair_cls.__name__, - ) - return message +{param_names_table} +See the help for `{altair_cls.__name__}` to read the full description of these parameters""" # Use the default error message for all other cases than unknown # parameter errors else: @@ -413,7 +411,7 @@ def _get_message_for_errors( if additional_errors: message += "\n" + "\n".join([e.message for e in additional_errors]) - return message + return message.strip() def _get_altair_class_for_error( self, error: jsonschema.exceptions.ValidationError From 879cb44fbcc821714911024a943cac0d25a68239 Mon Sep 17 00:00:00 2001 From: Stefan Binder Date: Mon, 10 Apr 2023 09:06:27 +0000 Subject: [PATCH 17/37] Reorder methods in calling order --- altair/utils/schemapi.py | 118 ++++++++++++++++++------------------- tools/schemapi/schemapi.py | 118 ++++++++++++++++++------------------- 2 files changed, 118 insertions(+), 118 deletions(-) diff --git a/altair/utils/schemapi.py b/altair/utils/schemapi.py index 3ce499515..03d961acf 100644 --- a/altair/utils/schemapi.py +++ b/altair/utils/schemapi.py @@ -319,6 +319,65 @@ def _get_message(self) -> str: messages.append(self._get_message_for_errors(errors, altair_cls)) return ("\n" + "-" * 50 + "\n").join(messages) + def _get_altair_class_for_error( + self, error: jsonschema.exceptions.ValidationError + ) -> Type["SchemaBase"]: + # Try to get the lowest class possible in the chart hierarchy so + # it can be displayed in the error message. This should lead to more informative + # error messages pointing the user closer to the source of the issue. + for prop_name in reversed(error.absolute_path): + # Check if str as e.g. first item can be a 0 + if isinstance(prop_name, str): + potential_class_name = prop_name[0].upper() + prop_name[1:] + cls = getattr(vegalite, potential_class_name, None) + if cls is not None: + break + else: + # Did not find a suitable class based on traversing the path so we fall + # back on the class of the top-level object which created + # the SchemaValidationError + cls = self.obj.__class__ + return cls + + def _get_message_for_errors( + self, + errors: Sequence[jsonschema.exceptions.ValidationError], + altair_cls: Type["SchemaBase"], + ) -> str: + error = errors[0] + # Output all existing parameters when an unknown parameter is specified + if error.validator == "additionalProperties": + param_dict_keys = inspect.signature(altair_cls).parameters.keys() + param_names_table = self._format_params_as_table(param_dict_keys) + + # Error messages for these errors look like this: + # "Additional properties are not allowed ('unknown' was unexpected)" + # Line below extracts "unknown" from this string + parameter_name = error.message.split("('")[-1].split("'")[0] + message = f"""\ +`{altair_cls.__name__}` has no parameter named '{parameter_name}' + +Existing parameter names are: +{param_names_table} +See the help for `{altair_cls.__name__}` to read the full description of these parameters""" + # Use the default error message for all other cases than unknown + # parameter errors + else: + message = error.message + # Add a summary line when parameters are passed an invalid value + # For example: "'asdf' is an invalid value for `stack` + if error.absolute_path: + message = f"""\ +'{error.instance}' is an invalid value for `{error.absolute_path[-1]}`: + +{message}""" + + additional_errors = [err for err in errors if err is not error] + if additional_errors: + message += "\n" + "\n".join([e.message for e in additional_errors]) + + return message.strip() + @staticmethod def _format_params_as_table(param_dict_keys: Iterable[str]) -> str: """Format param names into a table so that they are easier to read""" @@ -376,65 +435,6 @@ def split_into_equal_parts(n: int, p: int) -> List[int]: param_names_table += "\n" return param_names_table - def _get_message_for_errors( - self, - errors: Sequence[jsonschema.exceptions.ValidationError], - altair_cls: Type["SchemaBase"], - ) -> str: - error = errors[0] - # Output all existing parameters when an unknown parameter is specified - if error.validator == "additionalProperties": - param_dict_keys = inspect.signature(altair_cls).parameters.keys() - param_names_table = self._format_params_as_table(param_dict_keys) - - # Error messages for these errors look like this: - # "Additional properties are not allowed ('unknown' was unexpected)" - # Line below extracts "unknown" from this string - parameter_name = error.message.split("('")[-1].split("'")[0] - message = f"""\ -`{altair_cls.__name__}` has no parameter named '{parameter_name}' - -Existing parameter names are: -{param_names_table} -See the help for `{altair_cls.__name__}` to read the full description of these parameters""" - # Use the default error message for all other cases than unknown - # parameter errors - else: - message = error.message - # Add a summary line when parameters are passed an invalid value - # For example: "'asdf' is an invalid value for `stack` - if error.absolute_path: - message = f"""\ -'{error.instance}' is an invalid value for `{error.absolute_path[-1]}`: - -{message}""" - - additional_errors = [err for err in errors if err is not error] - if additional_errors: - message += "\n" + "\n".join([e.message for e in additional_errors]) - - return message.strip() - - def _get_altair_class_for_error( - self, error: jsonschema.exceptions.ValidationError - ) -> Type["SchemaBase"]: - # Try to get the lowest class possible in the chart hierarchy so - # it can be displayed in the error message. This should lead to more informative - # error messages pointing the user closer to the source of the issue. - for prop_name in reversed(error.absolute_path): - # Check if str as e.g. first item can be a 0 - if isinstance(prop_name, str): - potential_class_name = prop_name[0].upper() + prop_name[1:] - cls = getattr(vegalite, potential_class_name, None) - if cls is not None: - break - else: - # Did not find a suitable class based on traversing the path so we fall - # back on the class of the top-level object which created - # the SchemaValidationError - cls = self.obj.__class__ - return cls - class UndefinedType: """A singleton object for marking undefined parameters""" diff --git a/tools/schemapi/schemapi.py b/tools/schemapi/schemapi.py index e11570b9e..fdeb97241 100644 --- a/tools/schemapi/schemapi.py +++ b/tools/schemapi/schemapi.py @@ -317,6 +317,65 @@ def _get_message(self) -> str: messages.append(self._get_message_for_errors(errors, altair_cls)) return ("\n" + "-" * 50 + "\n").join(messages) + def _get_altair_class_for_error( + self, error: jsonschema.exceptions.ValidationError + ) -> Type["SchemaBase"]: + # Try to get the lowest class possible in the chart hierarchy so + # it can be displayed in the error message. This should lead to more informative + # error messages pointing the user closer to the source of the issue. + for prop_name in reversed(error.absolute_path): + # Check if str as e.g. first item can be a 0 + if isinstance(prop_name, str): + potential_class_name = prop_name[0].upper() + prop_name[1:] + cls = getattr(vegalite, potential_class_name, None) + if cls is not None: + break + else: + # Did not find a suitable class based on traversing the path so we fall + # back on the class of the top-level object which created + # the SchemaValidationError + cls = self.obj.__class__ + return cls + + def _get_message_for_errors( + self, + errors: Sequence[jsonschema.exceptions.ValidationError], + altair_cls: Type["SchemaBase"], + ) -> str: + error = errors[0] + # Output all existing parameters when an unknown parameter is specified + if error.validator == "additionalProperties": + param_dict_keys = inspect.signature(altair_cls).parameters.keys() + param_names_table = self._format_params_as_table(param_dict_keys) + + # Error messages for these errors look like this: + # "Additional properties are not allowed ('unknown' was unexpected)" + # Line below extracts "unknown" from this string + parameter_name = error.message.split("('")[-1].split("'")[0] + message = f"""\ +`{altair_cls.__name__}` has no parameter named '{parameter_name}' + +Existing parameter names are: +{param_names_table} +See the help for `{altair_cls.__name__}` to read the full description of these parameters""" + # Use the default error message for all other cases than unknown + # parameter errors + else: + message = error.message + # Add a summary line when parameters are passed an invalid value + # For example: "'asdf' is an invalid value for `stack` + if error.absolute_path: + message = f"""\ +'{error.instance}' is an invalid value for `{error.absolute_path[-1]}`: + +{message}""" + + additional_errors = [err for err in errors if err is not error] + if additional_errors: + message += "\n" + "\n".join([e.message for e in additional_errors]) + + return message.strip() + @staticmethod def _format_params_as_table(param_dict_keys: Iterable[str]) -> str: """Format param names into a table so that they are easier to read""" @@ -374,65 +433,6 @@ def split_into_equal_parts(n: int, p: int) -> List[int]: param_names_table += "\n" return param_names_table - def _get_message_for_errors( - self, - errors: Sequence[jsonschema.exceptions.ValidationError], - altair_cls: Type["SchemaBase"], - ) -> str: - error = errors[0] - # Output all existing parameters when an unknown parameter is specified - if error.validator == "additionalProperties": - param_dict_keys = inspect.signature(altair_cls).parameters.keys() - param_names_table = self._format_params_as_table(param_dict_keys) - - # Error messages for these errors look like this: - # "Additional properties are not allowed ('unknown' was unexpected)" - # Line below extracts "unknown" from this string - parameter_name = error.message.split("('")[-1].split("'")[0] - message = f"""\ -`{altair_cls.__name__}` has no parameter named '{parameter_name}' - -Existing parameter names are: -{param_names_table} -See the help for `{altair_cls.__name__}` to read the full description of these parameters""" - # Use the default error message for all other cases than unknown - # parameter errors - else: - message = error.message - # Add a summary line when parameters are passed an invalid value - # For example: "'asdf' is an invalid value for `stack` - if error.absolute_path: - message = f"""\ -'{error.instance}' is an invalid value for `{error.absolute_path[-1]}`: - -{message}""" - - additional_errors = [err for err in errors if err is not error] - if additional_errors: - message += "\n" + "\n".join([e.message for e in additional_errors]) - - return message.strip() - - def _get_altair_class_for_error( - self, error: jsonschema.exceptions.ValidationError - ) -> Type["SchemaBase"]: - # Try to get the lowest class possible in the chart hierarchy so - # it can be displayed in the error message. This should lead to more informative - # error messages pointing the user closer to the source of the issue. - for prop_name in reversed(error.absolute_path): - # Check if str as e.g. first item can be a 0 - if isinstance(prop_name, str): - potential_class_name = prop_name[0].upper() + prop_name[1:] - cls = getattr(vegalite, potential_class_name, None) - if cls is not None: - break - else: - # Did not find a suitable class based on traversing the path so we fall - # back on the class of the top-level object which created - # the SchemaValidationError - cls = self.obj.__class__ - return cls - class UndefinedType: """A singleton object for marking undefined parameters""" From 1cf2e225a37a58ece96ba6b2e8ea72c7bbbd93f6 Mon Sep 17 00:00:00 2001 From: Stefan Binder Date: Mon, 10 Apr 2023 09:16:58 +0000 Subject: [PATCH 18/37] Refactor _get_message_for_errors --- altair/utils/schemapi.py | 64 ++++++++++++++++++++++++-------------- tools/schemapi/schemapi.py | 64 ++++++++++++++++++++++++-------------- 2 files changed, 80 insertions(+), 48 deletions(-) diff --git a/altair/utils/schemapi.py b/altair/utils/schemapi.py index 03d961acf..215c083d9 100644 --- a/altair/utils/schemapi.py +++ b/altair/utils/schemapi.py @@ -347,36 +347,33 @@ def _get_message_for_errors( error = errors[0] # Output all existing parameters when an unknown parameter is specified if error.validator == "additionalProperties": - param_dict_keys = inspect.signature(altair_cls).parameters.keys() - param_names_table = self._format_params_as_table(param_dict_keys) + message = self._get_additional_properties_error_message( + altair_cls=altair_cls, error=error + ) + else: + message = self._get_default_error_message(error=error, errors=errors) - # Error messages for these errors look like this: - # "Additional properties are not allowed ('unknown' was unexpected)" - # Line below extracts "unknown" from this string - parameter_name = error.message.split("('")[-1].split("'")[0] - message = f"""\ + return message.strip() + + def _get_additional_properties_error_message( + self, + altair_cls: Type["SchemaBase"], + error: jsonschema.exceptions.ValidationError, + ) -> str: + param_dict_keys = inspect.signature(altair_cls).parameters.keys() + param_names_table = self._format_params_as_table(param_dict_keys) + + # Error messages for these errors look like this: + # "Additional properties are not allowed ('unknown' was unexpected)" + # Line below extracts "unknown" from this string + parameter_name = error.message.split("('")[-1].split("'")[0] + message = f"""\ `{altair_cls.__name__}` has no parameter named '{parameter_name}' Existing parameter names are: {param_names_table} See the help for `{altair_cls.__name__}` to read the full description of these parameters""" - # Use the default error message for all other cases than unknown - # parameter errors - else: - message = error.message - # Add a summary line when parameters are passed an invalid value - # For example: "'asdf' is an invalid value for `stack` - if error.absolute_path: - message = f"""\ -'{error.instance}' is an invalid value for `{error.absolute_path[-1]}`: - -{message}""" - - additional_errors = [err for err in errors if err is not error] - if additional_errors: - message += "\n" + "\n".join([e.message for e in additional_errors]) - - return message.strip() + return message @staticmethod def _format_params_as_table(param_dict_keys: Iterable[str]) -> str: @@ -435,6 +432,25 @@ def split_into_equal_parts(n: int, p: int) -> List[int]: param_names_table += "\n" return param_names_table + def _get_default_error_message( + self, + error: jsonschema.exceptions.ValidationError, + errors: Sequence[jsonschema.exceptions.ValidationError], + ) -> str: + message = error.message + # Add a summary line when parameters are passed an invalid value + # For example: "'asdf' is an invalid value for `stack` + if error.absolute_path: + message = f"""\ +'{error.instance}' is an invalid value for `{error.absolute_path[-1]}`: + +{message}""" + + additional_errors = [err for err in errors if err is not error] + if additional_errors: + message += "\n" + "\n".join([e.message for e in additional_errors]) + return message + class UndefinedType: """A singleton object for marking undefined parameters""" diff --git a/tools/schemapi/schemapi.py b/tools/schemapi/schemapi.py index fdeb97241..05fd529e1 100644 --- a/tools/schemapi/schemapi.py +++ b/tools/schemapi/schemapi.py @@ -345,36 +345,33 @@ def _get_message_for_errors( error = errors[0] # Output all existing parameters when an unknown parameter is specified if error.validator == "additionalProperties": - param_dict_keys = inspect.signature(altair_cls).parameters.keys() - param_names_table = self._format_params_as_table(param_dict_keys) + message = self._get_additional_properties_error_message( + altair_cls=altair_cls, error=error + ) + else: + message = self._get_default_error_message(error=error, errors=errors) - # Error messages for these errors look like this: - # "Additional properties are not allowed ('unknown' was unexpected)" - # Line below extracts "unknown" from this string - parameter_name = error.message.split("('")[-1].split("'")[0] - message = f"""\ + return message.strip() + + def _get_additional_properties_error_message( + self, + altair_cls: Type["SchemaBase"], + error: jsonschema.exceptions.ValidationError, + ) -> str: + param_dict_keys = inspect.signature(altair_cls).parameters.keys() + param_names_table = self._format_params_as_table(param_dict_keys) + + # Error messages for these errors look like this: + # "Additional properties are not allowed ('unknown' was unexpected)" + # Line below extracts "unknown" from this string + parameter_name = error.message.split("('")[-1].split("'")[0] + message = f"""\ `{altair_cls.__name__}` has no parameter named '{parameter_name}' Existing parameter names are: {param_names_table} See the help for `{altair_cls.__name__}` to read the full description of these parameters""" - # Use the default error message for all other cases than unknown - # parameter errors - else: - message = error.message - # Add a summary line when parameters are passed an invalid value - # For example: "'asdf' is an invalid value for `stack` - if error.absolute_path: - message = f"""\ -'{error.instance}' is an invalid value for `{error.absolute_path[-1]}`: - -{message}""" - - additional_errors = [err for err in errors if err is not error] - if additional_errors: - message += "\n" + "\n".join([e.message for e in additional_errors]) - - return message.strip() + return message @staticmethod def _format_params_as_table(param_dict_keys: Iterable[str]) -> str: @@ -433,6 +430,25 @@ def split_into_equal_parts(n: int, p: int) -> List[int]: param_names_table += "\n" return param_names_table + def _get_default_error_message( + self, + error: jsonschema.exceptions.ValidationError, + errors: Sequence[jsonschema.exceptions.ValidationError], + ) -> str: + message = error.message + # Add a summary line when parameters are passed an invalid value + # For example: "'asdf' is an invalid value for `stack` + if error.absolute_path: + message = f"""\ +'{error.instance}' is an invalid value for `{error.absolute_path[-1]}`: + +{message}""" + + additional_errors = [err for err in errors if err is not error] + if additional_errors: + message += "\n" + "\n".join([e.message for e in additional_errors]) + return message + class UndefinedType: """A singleton object for marking undefined parameters""" From 26f1a03fa9ad6d6ac35cbf21675dcb453534d945 Mon Sep 17 00:00:00 2001 From: Stefan Binder Date: Mon, 10 Apr 2023 14:59:59 +0000 Subject: [PATCH 19/37] Improve error messages for multiple errors about the same instance --- altair/utils/schemapi.py | 41 ++++++++++++++---- tests/utils/test_schemapi.py | 83 +++++++++++++----------------------- tools/schemapi/schemapi.py | 41 ++++++++++++++---- 3 files changed, 93 insertions(+), 72 deletions(-) diff --git a/altair/utils/schemapi.py b/altair/utils/schemapi.py index 215c083d9..0237f5bee 100644 --- a/altair/utils/schemapi.py +++ b/altair/utils/schemapi.py @@ -341,7 +341,7 @@ def _get_altair_class_for_error( def _get_message_for_errors( self, - errors: Sequence[jsonschema.exceptions.ValidationError], + errors: ValidationErrorList, altair_cls: Type["SchemaBase"], ) -> str: error = errors[0] @@ -435,20 +435,43 @@ def split_into_equal_parts(n: int, p: int) -> List[int]: def _get_default_error_message( self, error: jsonschema.exceptions.ValidationError, - errors: Sequence[jsonschema.exceptions.ValidationError], + errors: ValidationErrorList, ) -> str: - message = error.message + bullet_points: List[str] = [] + errors_by_validator = _group_errors_by_validator(errors) + if "enum" in errors_by_validator: + for error in errors_by_validator["enum"]: + bullet_points.append(f"one of {error.validator_value}") + + if "type" in errors_by_validator: + types = [f"'{err.validator_value}'" for err in errors_by_validator["type"]] + point = "of type " + if len(types) == 1: + point += types[0] + elif len(types) == 2: + point += f"{types[0]} or {types[1]}" + else: + point += ", ".join(types[:-1]) + f", or {types[-1]}" + bullet_points.append(point) + # Add a summary line when parameters are passed an invalid value # For example: "'asdf' is an invalid value for `stack` + message = f"'{error.instance}' is an invalid value" if error.absolute_path: - message = f"""\ -'{error.instance}' is an invalid value for `{error.absolute_path[-1]}`: + message += f" for `{error.absolute_path[-1]}`" + if len(bullet_points) == 0: + message += ".\n\n" + elif len(bullet_points) == 1: + message += f". Valid values are {bullet_points[0]}.\n\n" + else: + message += ". Valid values are:\n\n" + message += "\n".join([f"- {point}" for point in bullet_points]) + message += "\n\n" -{message}""" + for validator, errors in errors_by_validator.items(): + if validator not in ("enum", "type"): + message += "\n".join([e.message for e in errors]) - additional_errors = [err for err in errors if err is not error] - if additional_errors: - message += "\n" + "\n".join([e.message for e in additional_errors]) return message diff --git a/tests/utils/test_schemapi.py b/tests/utils/test_schemapi.py index 5c516a428..aa282e889 100644 --- a/tests/utils/test_schemapi.py +++ b/tests/utils/test_schemapi.py @@ -562,20 +562,16 @@ def chart_error_two_errors_in_complex_concat_layered_chart(): See the help for `X` to read the full description of these parameters -------------------------------------------------- - 'asdf' is an invalid value for `stack`: + 'asdf' is an invalid value for `stack`. Valid values are: - 'asdf' is not one of \['zero', 'center', 'normalize'\] - 'asdf' is not of type 'null' - 'asdf' is not of type 'boolean'$""" # noqa: W291 + - one of \['zero', 'center', 'normalize'\] + - of type 'null' or 'boolean'$""" # noqa: W291 ), ), ( chart_error_two_errors_in_layered_chart, inspect.cleandoc( - r"""'{'wrong'}' is an invalid value for `field`: - - {'wrong'} is not of type 'string' - {'wrong'} is not of type 'object' + r"""'{'wrong'}' is an invalid value for `field`. Valid values are of type 'string' or 'object'. -------------------------------------------------- `Encoding` has no parameter named 'invalidChannel' @@ -594,15 +590,10 @@ def chart_error_two_errors_in_complex_concat_layered_chart(): ( chart_error_two_errors_in_complex_concat_layered_chart, inspect.cleandoc( - r"""'{'wrong'}' is an invalid value for `field`: - - {'wrong'} is not of type 'string' - {'wrong'} is not of type 'object' + r"""'{'wrong'}' is an invalid value for `field`. Valid values are of type 'string' or 'object'. -------------------------------------------------- - '4' is an invalid value for `bandPosition`: - - '4' is not of type 'number'$""" - ) + '4' is an invalid value for `bandPosition`. Valid values are of type 'number'.$""" + ), ), ( chart_error_example_layer, @@ -621,31 +612,25 @@ def chart_error_two_errors_in_complex_concat_layered_chart(): ( chart_error_example_invalid_y_option_value, inspect.cleandoc( - r"""'asdf' is an invalid value for `stack`: + r"""'asdf' is an invalid value for `stack`. Valid values are: - 'asdf' is not one of \['zero', 'center', 'normalize'\] - 'asdf' is not of type 'null' - 'asdf' is not of type 'boolean'$""" + - one of \['zero', 'center', 'normalize'\] + - of type 'null' or 'boolean'$""" ), ), ( chart_error_example_invalid_y_option_value_with_condition, inspect.cleandoc( - r"""'asdf' is an invalid value for `stack`: + r"""'asdf' is an invalid value for `stack`. Valid values are: - 'asdf' is not one of \['zero', 'center', 'normalize'\] - 'asdf' is not of type 'null' - 'asdf' is not of type 'boolean'$""" + - one of \['zero', 'center', 'normalize'\] + - of type 'null' or 'boolean'$""" ), ), ( chart_error_example_hconcat, inspect.cleandoc( - r"""'{'text': 'Horsepower', 'align': 'right'}' is an invalid value for `title`: - - {'text': 'Horsepower', 'align': 'right'} is not of type 'string' - {'text': 'Horsepower', 'align': 'right'} is not of type 'array' - {'text': 'Horsepower', 'align': 'right'} is not of type 'null'$""" + r"""'{'text': 'Horsepower', 'align': 'right'}' is an invalid value for `title`. Valid values are of type 'string', 'array', or 'null'.$""" ), ), ( @@ -668,42 +653,36 @@ def chart_error_two_errors_in_complex_concat_layered_chart(): ( chart_error_example_invalid_timeunit_value, inspect.cleandoc( - r"""'invalid_value' is an invalid value for `timeUnit`: + r"""'invalid_value' is an invalid value for `timeUnit`. Valid values are: - 'invalid_value' is not one of \['year', 'quarter', 'month', 'week', 'day', 'dayofyear', 'date', 'hours', 'minutes', 'seconds', 'milliseconds'\] - 'invalid_value' is not one of \['utcyear', 'utcquarter', 'utcmonth', 'utcweek', 'utcday', 'utcdayofyear', 'utcdate', 'utchours', 'utcminutes', 'utcseconds', 'utcmilliseconds'\] - 'invalid_value' is not one of \['yearquarter', 'yearquartermonth', 'yearmonth', 'yearmonthdate', 'yearmonthdatehours', 'yearmonthdatehoursminutes', 'yearmonthdatehoursminutesseconds', 'yearweek', 'yearweekday', 'yearweekdayhours', 'yearweekdayhoursminutes', 'yearweekdayhoursminutesseconds', 'yeardayofyear', 'quartermonth', 'monthdate', 'monthdatehours', 'monthdatehoursminutes', 'monthdatehoursminutesseconds', 'weekday', 'weeksdayhours', 'weekdayhoursminutes', 'weekdayhoursminutesseconds', 'dayhours', 'dayhoursminutes', 'dayhoursminutesseconds', 'hoursminutes', 'hoursminutesseconds', 'minutesseconds', 'secondsmilliseconds'\] - 'invalid_value' is not one of \['utcyearquarter', 'utcyearquartermonth', 'utcyearmonth', 'utcyearmonthdate', 'utcyearmonthdatehours', 'utcyearmonthdatehoursminutes', 'utcyearmonthdatehoursminutesseconds', 'utcyearweek', 'utcyearweekday', 'utcyearweekdayhours', 'utcyearweekdayhoursminutes', 'utcyearweekdayhoursminutesseconds', 'utcyeardayofyear', 'utcquartermonth', 'utcmonthdate', 'utcmonthdatehours', 'utcmonthdatehoursminutes', 'utcmonthdatehoursminutesseconds', 'utcweekday', 'utcweeksdayhours', 'utcweekdayhoursminutes', 'utcweekdayhoursminutesseconds', 'utcdayhours', 'utcdayhoursminutes', 'utcdayhoursminutesseconds', 'utchoursminutes', 'utchoursminutesseconds', 'utcminutesseconds', 'utcsecondsmilliseconds'\] - 'invalid_value' is not of type 'object'$""" + - one of \['year', 'quarter', 'month', 'week', 'day', 'dayofyear', 'date', 'hours', 'minutes', 'seconds', 'milliseconds'\] + - one of \['utcyear', 'utcquarter', 'utcmonth', 'utcweek', 'utcday', 'utcdayofyear', 'utcdate', 'utchours', 'utcminutes', 'utcseconds', 'utcmilliseconds'\] + - one of \['yearquarter', 'yearquartermonth', 'yearmonth', 'yearmonthdate', 'yearmonthdatehours', 'yearmonthdatehoursminutes', 'yearmonthdatehoursminutesseconds', 'yearweek', 'yearweekday', 'yearweekdayhours', 'yearweekdayhoursminutes', 'yearweekdayhoursminutesseconds', 'yeardayofyear', 'quartermonth', 'monthdate', 'monthdatehours', 'monthdatehoursminutes', 'monthdatehoursminutesseconds', 'weekday', 'weeksdayhours', 'weekdayhoursminutes', 'weekdayhoursminutesseconds', 'dayhours', 'dayhoursminutes', 'dayhoursminutesseconds', 'hoursminutes', 'hoursminutesseconds', 'minutesseconds', 'secondsmilliseconds'\] + - one of \['utcyearquarter', 'utcyearquartermonth', 'utcyearmonth', 'utcyearmonthdate', 'utcyearmonthdatehours', 'utcyearmonthdatehoursminutes', 'utcyearmonthdatehoursminutesseconds', 'utcyearweek', 'utcyearweekday', 'utcyearweekdayhours', 'utcyearweekdayhoursminutes', 'utcyearweekdayhoursminutesseconds', 'utcyeardayofyear', 'utcquartermonth', 'utcmonthdate', 'utcmonthdatehours', 'utcmonthdatehoursminutes', 'utcmonthdatehoursminutesseconds', 'utcweekday', 'utcweeksdayhours', 'utcweekdayhoursminutes', 'utcweekdayhoursminutesseconds', 'utcdayhours', 'utcdayhoursminutes', 'utcdayhoursminutesseconds', 'utchoursminutes', 'utchoursminutesseconds', 'utcminutesseconds', 'utcsecondsmilliseconds'\] + - of type 'object'$""" ), ), ( chart_error_example_invalid_sort_value, inspect.cleandoc( - r"""'invalid_value' is an invalid value for `sort`: - - 'invalid_value' is not of type 'array' - 'invalid_value' is not of type 'object' - 'invalid_value' is not of type 'null' - 'invalid_value' is not one of \['ascending', 'descending'\] - 'invalid_value' is not one of \['x', 'y', 'color', 'fill', 'stroke', 'strokeWidth', 'size', 'shape', 'fillOpacity', 'strokeOpacity', 'opacity', 'text'\] - 'invalid_value' is not one of \['-x', '-y', '-color', '-fill', '-stroke', '-strokeWidth', '-size', '-shape', '-fillOpacity', '-strokeOpacity', '-opacity', '-text'\]$""" + r"""'invalid_value' is an invalid value for `sort`. Valid values are: + + - one of \['ascending', 'descending'\] + - one of \['x', 'y', 'color', 'fill', 'stroke', 'strokeWidth', 'size', 'shape', 'fillOpacity', 'strokeOpacity', 'opacity', 'text'\] + - one of \['-x', '-y', '-color', '-fill', '-stroke', '-strokeWidth', '-size', '-shape', '-fillOpacity', '-strokeOpacity', '-opacity', '-text'\] + - of type 'array', 'object', or 'null'$""" ), ), ( chart_error_example_invalid_bandposition_value, inspect.cleandoc( - r"""'4' is an invalid value for `bandPosition`: - - '4' is not of type 'number'$""" + r"""'4' is an invalid value for `bandPosition`. Valid values are of type 'number'.$""" ), ), ( chart_error_invalid_type, inspect.cleandoc( - r"""'unknown' is an invalid value for `type`: - - 'unknown' is not one of \['quantitative', 'ordinal', 'temporal', 'nominal', 'geojson'\]$""" + r"""'unknown' is an invalid value for `type`. Valid values are one of \['quantitative', 'ordinal', 'temporal', 'nominal', 'geojson'\].$""" ), ), ( @@ -723,11 +702,7 @@ def chart_error_two_errors_in_complex_concat_layered_chart(): ( chart_error_invalid_value_type, inspect.cleandoc( - r"""'1' is an invalid value for `value`: - - 1 is not of type 'object' - 1 is not of type 'string' - 1 is not of type 'null'$""" + r"""'1' is an invalid value for `value`. Valid values are of type 'object', 'string', or 'null'.$""" ), ), ], diff --git a/tools/schemapi/schemapi.py b/tools/schemapi/schemapi.py index 05fd529e1..5ed25175b 100644 --- a/tools/schemapi/schemapi.py +++ b/tools/schemapi/schemapi.py @@ -339,7 +339,7 @@ def _get_altair_class_for_error( def _get_message_for_errors( self, - errors: Sequence[jsonschema.exceptions.ValidationError], + errors: ValidationErrorList, altair_cls: Type["SchemaBase"], ) -> str: error = errors[0] @@ -433,20 +433,43 @@ def split_into_equal_parts(n: int, p: int) -> List[int]: def _get_default_error_message( self, error: jsonschema.exceptions.ValidationError, - errors: Sequence[jsonschema.exceptions.ValidationError], + errors: ValidationErrorList, ) -> str: - message = error.message + bullet_points: List[str] = [] + errors_by_validator = _group_errors_by_validator(errors) + if "enum" in errors_by_validator: + for error in errors_by_validator["enum"]: + bullet_points.append(f"one of {error.validator_value}") + + if "type" in errors_by_validator: + types = [f"'{err.validator_value}'" for err in errors_by_validator["type"]] + point = "of type " + if len(types) == 1: + point += types[0] + elif len(types) == 2: + point += f"{types[0]} or {types[1]}" + else: + point += ", ".join(types[:-1]) + f", or {types[-1]}" + bullet_points.append(point) + # Add a summary line when parameters are passed an invalid value # For example: "'asdf' is an invalid value for `stack` + message = f"'{error.instance}' is an invalid value" if error.absolute_path: - message = f"""\ -'{error.instance}' is an invalid value for `{error.absolute_path[-1]}`: + message += f" for `{error.absolute_path[-1]}`" + if len(bullet_points) == 0: + message += ".\n\n" + elif len(bullet_points) == 1: + message += f". Valid values are {bullet_points[0]}.\n\n" + else: + message += ". Valid values are:\n\n" + message += "\n".join([f"- {point}" for point in bullet_points]) + message += "\n\n" -{message}""" + for validator, errors in errors_by_validator.items(): + if validator not in ("enum", "type"): + message += "\n".join([e.message for e in errors]) - additional_errors = [err for err in errors if err is not error] - if additional_errors: - message += "\n" + "\n".join([e.message for e in additional_errors]) return message From 65b35b035ec4b96cd662667f94d6c90c3dea88c7 Mon Sep 17 00:00:00 2001 From: Stefan Binder Date: Tue, 11 Apr 2023 05:04:43 +0000 Subject: [PATCH 20/37] Move call of _get_altair_class_for_error into _get_message_for_errors --- altair/utils/schemapi.py | 35 +++++++++++++++++------------------ tools/schemapi/schemapi.py | 35 +++++++++++++++++------------------ 2 files changed, 34 insertions(+), 36 deletions(-) diff --git a/altair/utils/schemapi.py b/altair/utils/schemapi.py index 0237f5bee..91cc75b58 100644 --- a/altair/utils/schemapi.py +++ b/altair/utils/schemapi.py @@ -315,10 +315,25 @@ def __str__(self) -> str: def _get_message(self) -> str: messages: List[str] = [] for _, errors in self._errors.items(): - altair_cls = self._get_altair_class_for_error(errors[0]) - messages.append(self._get_message_for_errors(errors, altair_cls)) + messages.append(self._get_message_for_errors(errors)) return ("\n" + "-" * 50 + "\n").join(messages) + def _get_message_for_errors( + self, + errors: ValidationErrorList, + ) -> str: + error = errors[0] + altair_cls = self._get_altair_class_for_error(error) + # Output all existing parameters when an unknown parameter is specified + if error.validator == "additionalProperties": + message = self._get_additional_properties_error_message( + altair_cls=altair_cls, error=error + ) + else: + message = self._get_default_error_message(error=error, errors=errors) + + return message.strip() + def _get_altair_class_for_error( self, error: jsonschema.exceptions.ValidationError ) -> Type["SchemaBase"]: @@ -339,22 +354,6 @@ def _get_altair_class_for_error( cls = self.obj.__class__ return cls - def _get_message_for_errors( - self, - errors: ValidationErrorList, - altair_cls: Type["SchemaBase"], - ) -> str: - error = errors[0] - # Output all existing parameters when an unknown parameter is specified - if error.validator == "additionalProperties": - message = self._get_additional_properties_error_message( - altair_cls=altair_cls, error=error - ) - else: - message = self._get_default_error_message(error=error, errors=errors) - - return message.strip() - def _get_additional_properties_error_message( self, altair_cls: Type["SchemaBase"], diff --git a/tools/schemapi/schemapi.py b/tools/schemapi/schemapi.py index 5ed25175b..82888ef26 100644 --- a/tools/schemapi/schemapi.py +++ b/tools/schemapi/schemapi.py @@ -313,10 +313,25 @@ def __str__(self) -> str: def _get_message(self) -> str: messages: List[str] = [] for _, errors in self._errors.items(): - altair_cls = self._get_altair_class_for_error(errors[0]) - messages.append(self._get_message_for_errors(errors, altair_cls)) + messages.append(self._get_message_for_errors(errors)) return ("\n" + "-" * 50 + "\n").join(messages) + def _get_message_for_errors( + self, + errors: ValidationErrorList, + ) -> str: + error = errors[0] + altair_cls = self._get_altair_class_for_error(error) + # Output all existing parameters when an unknown parameter is specified + if error.validator == "additionalProperties": + message = self._get_additional_properties_error_message( + altair_cls=altair_cls, error=error + ) + else: + message = self._get_default_error_message(error=error, errors=errors) + + return message.strip() + def _get_altair_class_for_error( self, error: jsonschema.exceptions.ValidationError ) -> Type["SchemaBase"]: @@ -337,22 +352,6 @@ def _get_altair_class_for_error( cls = self.obj.__class__ return cls - def _get_message_for_errors( - self, - errors: ValidationErrorList, - altair_cls: Type["SchemaBase"], - ) -> str: - error = errors[0] - # Output all existing parameters when an unknown parameter is specified - if error.validator == "additionalProperties": - message = self._get_additional_properties_error_message( - altair_cls=altair_cls, error=error - ) - else: - message = self._get_default_error_message(error=error, errors=errors) - - return message.strip() - def _get_additional_properties_error_message( self, altair_cls: Type["SchemaBase"], From 058cba3819e8eaf85ff8f3343ef2203698da3443 Mon Sep 17 00:00:00 2001 From: Stefan Binder Date: Tue, 11 Apr 2023 05:38:55 +0000 Subject: [PATCH 21/37] Add docstrings --- altair/utils/schemapi.py | 117 ++++++++++++++++++++++++++----------- tools/schemapi/schemapi.py | 117 ++++++++++++++++++++++++++----------- 2 files changed, 166 insertions(+), 68 deletions(-) diff --git a/altair/utils/schemapi.py b/altair/utils/schemapi.py index 91cc75b58..d2204d690 100644 --- a/altair/utils/schemapi.py +++ b/altair/utils/schemapi.py @@ -65,6 +65,11 @@ def validate_jsonschema( rootschema: Optional[Dict[str, Any]] = None, raise_error: bool = True, ) -> Optional[jsonschema.exceptions.ValidationError]: + """Validates the passed in spec against the schema in the context of the + rootschema. If any errors are found, they are deduplicated and prioritized + and only the most relevant errors are kept. Errors are then either raised + or returned, depending on the value of `raise_error`. + """ errors = _get_errors_from_spec(spec, schema, rootschema=rootschema) if errors: leaf_errors = _get_leaves_of_error_tree(errors) @@ -75,8 +80,11 @@ def validate_jsonschema( # Nothing special about this first error but we need to choose one # which can be raised main_error = list(grouped_errors.values())[0][0] - # They can be used to craft more helpful error messages when this error - # is being converted to a SchemaValidationError + # All errors are then attached as a new attribute to ValidationError so that + # they can be used in SchemaValidationError to craft a more helpful + # error message. Setting a new attribute like this is not ideal as + # it then no longer matches the type ValidationError. It would be better + # to refactor this function to never raise but only return errors. main_error._all_errors = grouped_errors # type: ignore[attr-defined] if raise_error: raise main_error @@ -91,6 +99,11 @@ def _get_errors_from_spec( schema: Dict[str, Any], rootschema: Optional[Dict[str, Any]] = None, ) -> ValidationErrorList: + """Uses the relevant jsonschema validator to validate the passed in spec + against the schema using the rootschema to resolve references. + The schema and rootschema themselves are not validated but instead considered + as validate. + """ # We don't use jsonschema.validate as this would validate the schema itself. # Instead, we pass the schema directly to the validator class. This is done for # two reasons: The schema comes from Vega-Lite and is not based on the user @@ -119,6 +132,11 @@ def _get_errors_from_spec( def _group_errors_by_json_path( errors: ValidationErrorList, ) -> GroupedValidationErrors: + """Groups errors by the `json_path` attribute of the jsonschema ValidationError + class. This attribute contains the path to the offending element within + a chart specification and can therefore be considered as an identifier of an + 'issue' in the chart that needs to be fixed. + """ errors_by_json_path = collections.defaultdict(list) for err in errors: errors_by_json_path[err.json_path].append(err) @@ -128,9 +146,17 @@ def _group_errors_by_json_path( def _get_leaves_of_error_tree( errors: ValidationErrorList, ) -> ValidationErrorList: + """For each error in `errors`, it traverses down the "error tree" that is generated + by the jsonschema library to find and return all "leaf" errors. These are errors + which have no further errors that caused it and so they are the most specific errors + with the most specific error messages. + """ leaves: ValidationErrorList = [] for err in errors: if err.context: + # This means that the error `err` was caused by errors in subschemas. + # The list of errors from the subschemas are available in the property + # `context`. leaves.extend(_get_leaves_of_error_tree(err.context)) else: leaves.append(err) @@ -140,6 +166,12 @@ def _get_leaves_of_error_tree( def _subset_to_most_specific_json_paths( errors_by_json_path: GroupedValidationErrors, ) -> GroupedValidationErrors: + """Removes key (json path), value (errors) pairs where the json path is fully + contained in another json path. For example if `errors_by_json_path` has two + keys, `$.encoding.X` and `$.encoding.X.tooltip`, then the first one will be removed + and only the second one is returned. This is done under the assumption that + more specific json paths give more helpful error messages to the user. + """ errors_by_json_path_specific: GroupedValidationErrors = {} for json_path, errors in errors_by_json_path.items(): if not _contained_at_start_of_one_of_other_values( @@ -158,6 +190,10 @@ def _contained_at_start_of_one_of_other_values(x: str, values: Sequence[str]) -> def _deduplicate_errors( grouped_errors: GroupedValidationErrors, ) -> GroupedValidationErrors: + """Some errors have very similar error messages or are just in general not helpful + for a user. This function removes as many of these cases as possible and + can be extended over time to handle new cases that come up. + """ grouped_errors_deduplicated: GroupedValidationErrors = {} for json_path, element_errors in grouped_errors.items(): errors_by_validator = _group_errors_by_validator(element_errors) @@ -173,17 +209,16 @@ def _deduplicate_errors( errors = deduplication_func(errors) deduplicated_errors.extend(_deduplicate_by_message(errors)) - if len(deduplicated_errors) > 1: - # Removes any ValidationError "'value' is a required property" as these - # errors are unlikely to be the relevant ones for the user. They come from - # validation against a schema definition where the output of `alt.value` - # would be valid. However, if a user uses `alt.value`, the `value` keyword - # is included automatically from that function and so it's unlikely - # that this was what the user intended if the keyword is not present - # in the first place. - deduplicated_errors = [ - err for err in deduplicated_errors if not _is_required_value_error(err) - ] + # Removes any ValidationError "'value' is a required property" as these + # errors are unlikely to be the relevant ones for the user. They come from + # validation against a schema definition where the output of `alt.value` + # would be valid. However, if a user uses `alt.value`, the `value` keyword + # is included automatically from that function and so it's unlikely + # that this was what the user intended if the keyword is not present + # in the first place. + deduplicated_errors = [ + err for err in deduplicated_errors if not _is_required_value_error(err) + ] grouped_errors_deduplicated[json_path] = deduplicated_errors return grouped_errors_deduplicated @@ -194,6 +229,12 @@ def _is_required_value_error(err: jsonschema.exceptions.ValidationError) -> bool def _group_errors_by_validator(errors: ValidationErrorList) -> GroupedValidationErrors: + """Groups the errors by the json schema "validator" that casued the error. For + example if the error is that a value is not one of an enumeration in the json schema + then the "validator" is `"enum"`, if the error is due to an unknown property that + was set although no additional properties are allowed then "validator" is + `"additionalProperties`, etc. + """ errors_by_validator: DefaultDict[ str, ValidationErrorList ] = collections.defaultdict(list) @@ -205,13 +246,13 @@ def _group_errors_by_validator(errors: ValidationErrorList) -> GroupedValidation def _deduplicate_enum_errors(errors: ValidationErrorList) -> ValidationErrorList: + """Deduplicate enum errors by removing the errors where the allowed values + are a subset of another error. For example, if `enum` contains two errors + and one has `validator_value` (i.e. accepted values) ["A", "B"] and the + other one ["A", "B", "C"] then the first one is removed and the final + `enum` list only contains the error with ["A", "B", "C"]. + """ if len(errors) > 1: - # Deduplicate enum errors by removing the errors where the allowed values - # are a subset of another error. For example, if `enum` contains two errors - # and one has `validator_value` (i.e. accepted values) ["A", "B"] and the - # other one ["A", "B", "C"] then the first one is removed and the final - # `enum` list only contains the error with ["A", "B", "C"] - # Values (and therefore `validator_value`) of an enum are always arrays, # see https://json-schema.org/understanding-json-schema/reference/generic.html#enumerated-values # which is why we can use join below @@ -227,17 +268,18 @@ def _deduplicate_enum_errors(errors: ValidationErrorList) -> ValidationErrorList def _deduplicate_additional_properties_errors( errors: ValidationErrorList, ) -> ValidationErrorList: + """If there are multiple additional property errors it usually means that + the offending element was validated against multiple schemas and + its parent is a common anyOf validator. + The error messages produced from these cases are usually + very similar and we just take the shortest one. For example, + the following 3 errors are raised for the `unknown` channel option in + `alt.X("variety", unknown=2)`: + - "Additional properties are not allowed ('unknown' was unexpected)" + - "Additional properties are not allowed ('field', 'unknown' were unexpected)" + - "Additional properties are not allowed ('field', 'type', 'unknown' were unexpected)" + """ if len(errors) > 1: - # If there are multiple additional property errors it usually means that - # the offending element was validated against multiple schemas and - # its parent is a common anyOf validator. - # The error messages produced from these cases are usually - # very similar and we just take the shortest one. For example, - # the following 3 errors are raised for the `unknown` channel option in - # `alt.X("variety", unknown=2)`: - # - "Additional properties are not allowed ('unknown' was unexpected)" - # - "Additional properties are not allowed ('field', 'unknown' were unexpected)" - # - "Additional properties are not allowed ('field', 'type', 'unknown' were unexpected)" # The code below subsets this to only the first error of the three. parent = errors[0].parent # Test if all parent errors are the same anyOf error and only do @@ -253,8 +295,9 @@ def _deduplicate_additional_properties_errors( def _deduplicate_by_message(errors: ValidationErrorList) -> ValidationErrorList: - # Deduplicate errors by message. This keeps the original order in case - # it was chosen intentionally + """Deduplicate errors by message. This keeps the original order in case + it was chosen intentionally. + """ return list({e.message: e for e in errors}.values()) @@ -337,9 +380,10 @@ def _get_message_for_errors( def _get_altair_class_for_error( self, error: jsonschema.exceptions.ValidationError ) -> Type["SchemaBase"]: - # Try to get the lowest class possible in the chart hierarchy so - # it can be displayed in the error message. This should lead to more informative - # error messages pointing the user closer to the source of the issue. + """Try to get the lowest class possible in the chart hierarchy so + it can be displayed in the error message. This should lead to more informative + error messages pointing the user closer to the source of the issue. + """ for prop_name in reversed(error.absolute_path): # Check if str as e.g. first item can be a 0 if isinstance(prop_name, str): @@ -458,6 +502,8 @@ def _get_default_error_message( message = f"'{error.instance}' is an invalid value" if error.absolute_path: message += f" for `{error.absolute_path[-1]}`" + + # Add bullet points if len(bullet_points) == 0: message += ".\n\n" elif len(bullet_points) == 1: @@ -467,6 +513,9 @@ def _get_default_error_message( message += "\n".join([f"- {point}" for point in bullet_points]) message += "\n\n" + # Add unformatted messages of any remaining errors which were not + # considered so far. This is not expected to be used but more exists + # as a fallback for cases which were not known during development. for validator, errors in errors_by_validator.items(): if validator not in ("enum", "type"): message += "\n".join([e.message for e in errors]) diff --git a/tools/schemapi/schemapi.py b/tools/schemapi/schemapi.py index 82888ef26..cc71ca6cf 100644 --- a/tools/schemapi/schemapi.py +++ b/tools/schemapi/schemapi.py @@ -63,6 +63,11 @@ def validate_jsonschema( rootschema: Optional[Dict[str, Any]] = None, raise_error: bool = True, ) -> Optional[jsonschema.exceptions.ValidationError]: + """Validates the passed in spec against the schema in the context of the + rootschema. If any errors are found, they are deduplicated and prioritized + and only the most relevant errors are kept. Errors are then either raised + or returned, depending on the value of `raise_error`. + """ errors = _get_errors_from_spec(spec, schema, rootschema=rootschema) if errors: leaf_errors = _get_leaves_of_error_tree(errors) @@ -73,8 +78,11 @@ def validate_jsonschema( # Nothing special about this first error but we need to choose one # which can be raised main_error = list(grouped_errors.values())[0][0] - # They can be used to craft more helpful error messages when this error - # is being converted to a SchemaValidationError + # All errors are then attached as a new attribute to ValidationError so that + # they can be used in SchemaValidationError to craft a more helpful + # error message. Setting a new attribute like this is not ideal as + # it then no longer matches the type ValidationError. It would be better + # to refactor this function to never raise but only return errors. main_error._all_errors = grouped_errors # type: ignore[attr-defined] if raise_error: raise main_error @@ -89,6 +97,11 @@ def _get_errors_from_spec( schema: Dict[str, Any], rootschema: Optional[Dict[str, Any]] = None, ) -> ValidationErrorList: + """Uses the relevant jsonschema validator to validate the passed in spec + against the schema using the rootschema to resolve references. + The schema and rootschema themselves are not validated but instead considered + as validate. + """ # We don't use jsonschema.validate as this would validate the schema itself. # Instead, we pass the schema directly to the validator class. This is done for # two reasons: The schema comes from Vega-Lite and is not based on the user @@ -117,6 +130,11 @@ def _get_errors_from_spec( def _group_errors_by_json_path( errors: ValidationErrorList, ) -> GroupedValidationErrors: + """Groups errors by the `json_path` attribute of the jsonschema ValidationError + class. This attribute contains the path to the offending element within + a chart specification and can therefore be considered as an identifier of an + 'issue' in the chart that needs to be fixed. + """ errors_by_json_path = collections.defaultdict(list) for err in errors: errors_by_json_path[err.json_path].append(err) @@ -126,9 +144,17 @@ def _group_errors_by_json_path( def _get_leaves_of_error_tree( errors: ValidationErrorList, ) -> ValidationErrorList: + """For each error in `errors`, it traverses down the "error tree" that is generated + by the jsonschema library to find and return all "leaf" errors. These are errors + which have no further errors that caused it and so they are the most specific errors + with the most specific error messages. + """ leaves: ValidationErrorList = [] for err in errors: if err.context: + # This means that the error `err` was caused by errors in subschemas. + # The list of errors from the subschemas are available in the property + # `context`. leaves.extend(_get_leaves_of_error_tree(err.context)) else: leaves.append(err) @@ -138,6 +164,12 @@ def _get_leaves_of_error_tree( def _subset_to_most_specific_json_paths( errors_by_json_path: GroupedValidationErrors, ) -> GroupedValidationErrors: + """Removes key (json path), value (errors) pairs where the json path is fully + contained in another json path. For example if `errors_by_json_path` has two + keys, `$.encoding.X` and `$.encoding.X.tooltip`, then the first one will be removed + and only the second one is returned. This is done under the assumption that + more specific json paths give more helpful error messages to the user. + """ errors_by_json_path_specific: GroupedValidationErrors = {} for json_path, errors in errors_by_json_path.items(): if not _contained_at_start_of_one_of_other_values( @@ -156,6 +188,10 @@ def _contained_at_start_of_one_of_other_values(x: str, values: Sequence[str]) -> def _deduplicate_errors( grouped_errors: GroupedValidationErrors, ) -> GroupedValidationErrors: + """Some errors have very similar error messages or are just in general not helpful + for a user. This function removes as many of these cases as possible and + can be extended over time to handle new cases that come up. + """ grouped_errors_deduplicated: GroupedValidationErrors = {} for json_path, element_errors in grouped_errors.items(): errors_by_validator = _group_errors_by_validator(element_errors) @@ -171,17 +207,16 @@ def _deduplicate_errors( errors = deduplication_func(errors) deduplicated_errors.extend(_deduplicate_by_message(errors)) - if len(deduplicated_errors) > 1: - # Removes any ValidationError "'value' is a required property" as these - # errors are unlikely to be the relevant ones for the user. They come from - # validation against a schema definition where the output of `alt.value` - # would be valid. However, if a user uses `alt.value`, the `value` keyword - # is included automatically from that function and so it's unlikely - # that this was what the user intended if the keyword is not present - # in the first place. - deduplicated_errors = [ - err for err in deduplicated_errors if not _is_required_value_error(err) - ] + # Removes any ValidationError "'value' is a required property" as these + # errors are unlikely to be the relevant ones for the user. They come from + # validation against a schema definition where the output of `alt.value` + # would be valid. However, if a user uses `alt.value`, the `value` keyword + # is included automatically from that function and so it's unlikely + # that this was what the user intended if the keyword is not present + # in the first place. + deduplicated_errors = [ + err for err in deduplicated_errors if not _is_required_value_error(err) + ] grouped_errors_deduplicated[json_path] = deduplicated_errors return grouped_errors_deduplicated @@ -192,6 +227,12 @@ def _is_required_value_error(err: jsonschema.exceptions.ValidationError) -> bool def _group_errors_by_validator(errors: ValidationErrorList) -> GroupedValidationErrors: + """Groups the errors by the json schema "validator" that casued the error. For + example if the error is that a value is not one of an enumeration in the json schema + then the "validator" is `"enum"`, if the error is due to an unknown property that + was set although no additional properties are allowed then "validator" is + `"additionalProperties`, etc. + """ errors_by_validator: DefaultDict[ str, ValidationErrorList ] = collections.defaultdict(list) @@ -203,13 +244,13 @@ def _group_errors_by_validator(errors: ValidationErrorList) -> GroupedValidation def _deduplicate_enum_errors(errors: ValidationErrorList) -> ValidationErrorList: + """Deduplicate enum errors by removing the errors where the allowed values + are a subset of another error. For example, if `enum` contains two errors + and one has `validator_value` (i.e. accepted values) ["A", "B"] and the + other one ["A", "B", "C"] then the first one is removed and the final + `enum` list only contains the error with ["A", "B", "C"]. + """ if len(errors) > 1: - # Deduplicate enum errors by removing the errors where the allowed values - # are a subset of another error. For example, if `enum` contains two errors - # and one has `validator_value` (i.e. accepted values) ["A", "B"] and the - # other one ["A", "B", "C"] then the first one is removed and the final - # `enum` list only contains the error with ["A", "B", "C"] - # Values (and therefore `validator_value`) of an enum are always arrays, # see https://json-schema.org/understanding-json-schema/reference/generic.html#enumerated-values # which is why we can use join below @@ -225,17 +266,18 @@ def _deduplicate_enum_errors(errors: ValidationErrorList) -> ValidationErrorList def _deduplicate_additional_properties_errors( errors: ValidationErrorList, ) -> ValidationErrorList: + """If there are multiple additional property errors it usually means that + the offending element was validated against multiple schemas and + its parent is a common anyOf validator. + The error messages produced from these cases are usually + very similar and we just take the shortest one. For example, + the following 3 errors are raised for the `unknown` channel option in + `alt.X("variety", unknown=2)`: + - "Additional properties are not allowed ('unknown' was unexpected)" + - "Additional properties are not allowed ('field', 'unknown' were unexpected)" + - "Additional properties are not allowed ('field', 'type', 'unknown' were unexpected)" + """ if len(errors) > 1: - # If there are multiple additional property errors it usually means that - # the offending element was validated against multiple schemas and - # its parent is a common anyOf validator. - # The error messages produced from these cases are usually - # very similar and we just take the shortest one. For example, - # the following 3 errors are raised for the `unknown` channel option in - # `alt.X("variety", unknown=2)`: - # - "Additional properties are not allowed ('unknown' was unexpected)" - # - "Additional properties are not allowed ('field', 'unknown' were unexpected)" - # - "Additional properties are not allowed ('field', 'type', 'unknown' were unexpected)" # The code below subsets this to only the first error of the three. parent = errors[0].parent # Test if all parent errors are the same anyOf error and only do @@ -251,8 +293,9 @@ def _deduplicate_additional_properties_errors( def _deduplicate_by_message(errors: ValidationErrorList) -> ValidationErrorList: - # Deduplicate errors by message. This keeps the original order in case - # it was chosen intentionally + """Deduplicate errors by message. This keeps the original order in case + it was chosen intentionally. + """ return list({e.message: e for e in errors}.values()) @@ -335,9 +378,10 @@ def _get_message_for_errors( def _get_altair_class_for_error( self, error: jsonschema.exceptions.ValidationError ) -> Type["SchemaBase"]: - # Try to get the lowest class possible in the chart hierarchy so - # it can be displayed in the error message. This should lead to more informative - # error messages pointing the user closer to the source of the issue. + """Try to get the lowest class possible in the chart hierarchy so + it can be displayed in the error message. This should lead to more informative + error messages pointing the user closer to the source of the issue. + """ for prop_name in reversed(error.absolute_path): # Check if str as e.g. first item can be a 0 if isinstance(prop_name, str): @@ -456,6 +500,8 @@ def _get_default_error_message( message = f"'{error.instance}' is an invalid value" if error.absolute_path: message += f" for `{error.absolute_path[-1]}`" + + # Add bullet points if len(bullet_points) == 0: message += ".\n\n" elif len(bullet_points) == 1: @@ -465,6 +511,9 @@ def _get_default_error_message( message += "\n".join([f"- {point}" for point in bullet_points]) message += "\n\n" + # Add unformatted messages of any remaining errors which were not + # considered so far. This is not expected to be used but more exists + # as a fallback for cases which were not known during development. for validator, errors in errors_by_validator.items(): if validator not in ("enum", "type"): message += "\n".join([e.message for e in errors]) From 9e6fb2d2fd84ecd933af016bacd8077ca5dc65a7 Mon Sep 17 00:00:00 2001 From: Stefan Binder Date: Tue, 11 Apr 2023 05:48:17 +0000 Subject: [PATCH 22/37] Minor refactoring --- altair/utils/schemapi.py | 61 +++++++++++++++++++++----------------- tools/schemapi/schemapi.py | 61 +++++++++++++++++++++----------------- 2 files changed, 66 insertions(+), 56 deletions(-) diff --git a/altair/utils/schemapi.py b/altair/utils/schemapi.py index d2204d690..46f7be20d 100644 --- a/altair/utils/schemapi.py +++ b/altair/utils/schemapi.py @@ -365,18 +365,39 @@ def _get_message_for_errors( self, errors: ValidationErrorList, ) -> str: - error = errors[0] - altair_cls = self._get_altair_class_for_error(error) - # Output all existing parameters when an unknown parameter is specified - if error.validator == "additionalProperties": - message = self._get_additional_properties_error_message( - altair_cls=altair_cls, error=error - ) + if errors[0].validator == "additionalProperties": + # During development, we only found cases where an additionalProperties + # error was raised if that was the only error for the offending instance + # as identifiable by the json path. Therefore, we just check here the first + # error. However, other constellations might exist in which case + # this should be adapted so that other error messages are shown as well. + message = self._get_additional_properties_error_message(errors[0]) else: - message = self._get_default_error_message(error=error, errors=errors) + message = self._get_default_error_message(errors=errors) return message.strip() + def _get_additional_properties_error_message( + self, + error: jsonschema.exceptions.ValidationError, + ) -> str: + """Output all existing parameters when an unknown parameter is specified.""" + altair_cls = self._get_altair_class_for_error(error) + param_dict_keys = inspect.signature(altair_cls).parameters.keys() + param_names_table = self._format_params_as_table(param_dict_keys) + + # Error messages for these errors look like this: + # "Additional properties are not allowed ('unknown' was unexpected)" + # Line below extracts "unknown" from this string + parameter_name = error.message.split("('")[-1].split("'")[0] + message = f"""\ +`{altair_cls.__name__}` has no parameter named '{parameter_name}' + +Existing parameter names are: +{param_names_table} +See the help for `{altair_cls.__name__}` to read the full description of these parameters""" + return message + def _get_altair_class_for_error( self, error: jsonschema.exceptions.ValidationError ) -> Type["SchemaBase"]: @@ -398,25 +419,6 @@ def _get_altair_class_for_error( cls = self.obj.__class__ return cls - def _get_additional_properties_error_message( - self, - altair_cls: Type["SchemaBase"], - error: jsonschema.exceptions.ValidationError, - ) -> str: - param_dict_keys = inspect.signature(altair_cls).parameters.keys() - param_names_table = self._format_params_as_table(param_dict_keys) - - # Error messages for these errors look like this: - # "Additional properties are not allowed ('unknown' was unexpected)" - # Line below extracts "unknown" from this string - parameter_name = error.message.split("('")[-1].split("'")[0] - message = f"""\ -`{altair_cls.__name__}` has no parameter named '{parameter_name}' - -Existing parameter names are: -{param_names_table} -See the help for `{altair_cls.__name__}` to read the full description of these parameters""" - return message @staticmethod def _format_params_as_table(param_dict_keys: Iterable[str]) -> str: @@ -477,7 +479,6 @@ def split_into_equal_parts(n: int, p: int) -> List[int]: def _get_default_error_message( self, - error: jsonschema.exceptions.ValidationError, errors: ValidationErrorList, ) -> str: bullet_points: List[str] = [] @@ -497,6 +498,10 @@ def _get_default_error_message( point += ", ".join(types[:-1]) + f", or {types[-1]}" bullet_points.append(point) + # It should not matter which error is specifically used as they are all + # about the same offending instance (i.e. invalid value), so we can just + # take the first one + error = errors[0] # Add a summary line when parameters are passed an invalid value # For example: "'asdf' is an invalid value for `stack` message = f"'{error.instance}' is an invalid value" diff --git a/tools/schemapi/schemapi.py b/tools/schemapi/schemapi.py index cc71ca6cf..89d86c642 100644 --- a/tools/schemapi/schemapi.py +++ b/tools/schemapi/schemapi.py @@ -363,18 +363,39 @@ def _get_message_for_errors( self, errors: ValidationErrorList, ) -> str: - error = errors[0] - altair_cls = self._get_altair_class_for_error(error) - # Output all existing parameters when an unknown parameter is specified - if error.validator == "additionalProperties": - message = self._get_additional_properties_error_message( - altair_cls=altair_cls, error=error - ) + if errors[0].validator == "additionalProperties": + # During development, we only found cases where an additionalProperties + # error was raised if that was the only error for the offending instance + # as identifiable by the json path. Therefore, we just check here the first + # error. However, other constellations might exist in which case + # this should be adapted so that other error messages are shown as well. + message = self._get_additional_properties_error_message(errors[0]) else: - message = self._get_default_error_message(error=error, errors=errors) + message = self._get_default_error_message(errors=errors) return message.strip() + def _get_additional_properties_error_message( + self, + error: jsonschema.exceptions.ValidationError, + ) -> str: + """Output all existing parameters when an unknown parameter is specified.""" + altair_cls = self._get_altair_class_for_error(error) + param_dict_keys = inspect.signature(altair_cls).parameters.keys() + param_names_table = self._format_params_as_table(param_dict_keys) + + # Error messages for these errors look like this: + # "Additional properties are not allowed ('unknown' was unexpected)" + # Line below extracts "unknown" from this string + parameter_name = error.message.split("('")[-1].split("'")[0] + message = f"""\ +`{altair_cls.__name__}` has no parameter named '{parameter_name}' + +Existing parameter names are: +{param_names_table} +See the help for `{altair_cls.__name__}` to read the full description of these parameters""" + return message + def _get_altair_class_for_error( self, error: jsonschema.exceptions.ValidationError ) -> Type["SchemaBase"]: @@ -396,25 +417,6 @@ def _get_altair_class_for_error( cls = self.obj.__class__ return cls - def _get_additional_properties_error_message( - self, - altair_cls: Type["SchemaBase"], - error: jsonschema.exceptions.ValidationError, - ) -> str: - param_dict_keys = inspect.signature(altair_cls).parameters.keys() - param_names_table = self._format_params_as_table(param_dict_keys) - - # Error messages for these errors look like this: - # "Additional properties are not allowed ('unknown' was unexpected)" - # Line below extracts "unknown" from this string - parameter_name = error.message.split("('")[-1].split("'")[0] - message = f"""\ -`{altair_cls.__name__}` has no parameter named '{parameter_name}' - -Existing parameter names are: -{param_names_table} -See the help for `{altair_cls.__name__}` to read the full description of these parameters""" - return message @staticmethod def _format_params_as_table(param_dict_keys: Iterable[str]) -> str: @@ -475,7 +477,6 @@ def split_into_equal_parts(n: int, p: int) -> List[int]: def _get_default_error_message( self, - error: jsonschema.exceptions.ValidationError, errors: ValidationErrorList, ) -> str: bullet_points: List[str] = [] @@ -495,6 +496,10 @@ def _get_default_error_message( point += ", ".join(types[:-1]) + f", or {types[-1]}" bullet_points.append(point) + # It should not matter which error is specifically used as they are all + # about the same offending instance (i.e. invalid value), so we can just + # take the first one + error = errors[0] # Add a summary line when parameters are passed an invalid value # For example: "'asdf' is an invalid value for `stack` message = f"'{error.instance}' is an invalid value" From b777e4d9465f7234824a00aa912ed98222795023 Mon Sep 17 00:00:00 2001 From: Stefan Binder Date: Tue, 11 Apr 2023 05:49:00 +0000 Subject: [PATCH 23/37] Format code --- altair/utils/schemapi.py | 1 - tools/schemapi/schemapi.py | 1 - 2 files changed, 2 deletions(-) diff --git a/altair/utils/schemapi.py b/altair/utils/schemapi.py index 46f7be20d..270de4b88 100644 --- a/altair/utils/schemapi.py +++ b/altair/utils/schemapi.py @@ -419,7 +419,6 @@ def _get_altair_class_for_error( cls = self.obj.__class__ return cls - @staticmethod def _format_params_as_table(param_dict_keys: Iterable[str]) -> str: """Format param names into a table so that they are easier to read""" diff --git a/tools/schemapi/schemapi.py b/tools/schemapi/schemapi.py index 89d86c642..00fd39b56 100644 --- a/tools/schemapi/schemapi.py +++ b/tools/schemapi/schemapi.py @@ -417,7 +417,6 @@ def _get_altair_class_for_error( cls = self.obj.__class__ return cls - @staticmethod def _format_params_as_table(param_dict_keys: Iterable[str]) -> str: """Format param names into a table so that they are easier to read""" From 114c153e279f05ee960e64145313c0928f6061af Mon Sep 17 00:00:00 2001 From: Stefan Binder Date: Sat, 15 Apr 2023 06:10:07 +0000 Subject: [PATCH 24/37] Enumerate error messages. Don't separate by line as unclear how long it should be as this depends on the frontend --- altair/utils/schemapi.py | 16 +++++++++++----- tests/utils/test_schemapi.py | 18 +++++++++--------- tools/schemapi/schemapi.py | 16 +++++++++++----- 3 files changed, 31 insertions(+), 19 deletions(-) diff --git a/altair/utils/schemapi.py b/altair/utils/schemapi.py index 270de4b88..fe1f62dd3 100644 --- a/altair/utils/schemapi.py +++ b/altair/utils/schemapi.py @@ -356,12 +356,18 @@ def __str__(self) -> str: return self.message def _get_message(self) -> str: - messages: List[str] = [] - for _, errors in self._errors.items(): - messages.append(self._get_message_for_errors(errors)) - return ("\n" + "-" * 50 + "\n").join(messages) + error_messages: List[str] = [] + for errors in self._errors.values(): + error_messages.append(self._get_message_for_errors_group(errors)) + + if len(error_messages) > 1: + error_messages = [ + f"Error #{error_id}: {message}" + for error_id, message in enumerate(error_messages, start=1) + ] + return ("\n\n").join(error_messages) - def _get_message_for_errors( + def _get_message_for_errors_group( self, errors: ValidationErrorList, ) -> str: diff --git a/tests/utils/test_schemapi.py b/tests/utils/test_schemapi.py index aa282e889..7a53ca322 100644 --- a/tests/utils/test_schemapi.py +++ b/tests/utils/test_schemapi.py @@ -552,7 +552,7 @@ def chart_error_two_errors_in_complex_concat_layered_chart(): ( chart_error_example_invalid_y_option_value_unknown_x_option, inspect.cleandoc( - r"""`X` has no parameter named 'unknown' + r"""Error \#1: `X` has no parameter named 'unknown' Existing parameter names are: shorthand bin scale timeUnit @@ -561,8 +561,8 @@ def chart_error_two_errors_in_complex_concat_layered_chart(): bandPosition See the help for `X` to read the full description of these parameters - -------------------------------------------------- - 'asdf' is an invalid value for `stack`. Valid values are: + + Error \#2: 'asdf' is an invalid value for `stack`. Valid values are: - one of \['zero', 'center', 'normalize'\] - of type 'null' or 'boolean'$""" # noqa: W291 @@ -571,9 +571,9 @@ def chart_error_two_errors_in_complex_concat_layered_chart(): ( chart_error_two_errors_in_layered_chart, inspect.cleandoc( - r"""'{'wrong'}' is an invalid value for `field`. Valid values are of type 'string' or 'object'. - -------------------------------------------------- - `Encoding` has no parameter named 'invalidChannel' + r"""Error \#1: '{'wrong'}' is an invalid value for `field`. Valid values are of type 'string' or 'object'. + + Error \#2: `Encoding` has no parameter named 'invalidChannel' Existing parameter names are: angle key order strokeDash tooltip xOffset @@ -590,9 +590,9 @@ def chart_error_two_errors_in_complex_concat_layered_chart(): ( chart_error_two_errors_in_complex_concat_layered_chart, inspect.cleandoc( - r"""'{'wrong'}' is an invalid value for `field`. Valid values are of type 'string' or 'object'. - -------------------------------------------------- - '4' is an invalid value for `bandPosition`. Valid values are of type 'number'.$""" + r"""Error \#1: '{'wrong'}' is an invalid value for `field`. Valid values are of type 'string' or 'object'. + + Error \#2: '4' is an invalid value for `bandPosition`. Valid values are of type 'number'.$""" ), ), ( diff --git a/tools/schemapi/schemapi.py b/tools/schemapi/schemapi.py index 00fd39b56..983f92a6c 100644 --- a/tools/schemapi/schemapi.py +++ b/tools/schemapi/schemapi.py @@ -354,12 +354,18 @@ def __str__(self) -> str: return self.message def _get_message(self) -> str: - messages: List[str] = [] - for _, errors in self._errors.items(): - messages.append(self._get_message_for_errors(errors)) - return ("\n" + "-" * 50 + "\n").join(messages) + error_messages: List[str] = [] + for errors in self._errors.values(): + error_messages.append(self._get_message_for_errors_group(errors)) + + if len(error_messages) > 1: + error_messages = [ + f"Error #{error_id}: {message}" + for error_id, message in enumerate(error_messages, start=1) + ] + return ("\n\n").join(error_messages) - def _get_message_for_errors( + def _get_message_for_errors_group( self, errors: ValidationErrorList, ) -> str: From 2062468de5b05dff454a8f0950b06679af72dcb9 Mon Sep 17 00:00:00 2001 From: Stefan Binder Date: Sat, 15 Apr 2023 06:54:57 +0000 Subject: [PATCH 25/37] Add more test examples --- tests/utils/test_schemapi.py | 141 ++++++++++++++++++++++++++++++++++- 1 file changed, 137 insertions(+), 4 deletions(-) diff --git a/tests/utils/test_schemapi.py b/tests/utils/test_schemapi.py index 7a53ca322..abf34a236 100644 --- a/tests/utils/test_schemapi.py +++ b/tests/utils/test_schemapi.py @@ -527,6 +527,23 @@ def chart_error_invalid_value_type(): ) +def chart_error_wrong_tooltip_type_in_faceted_chart(): + # Error: Wrong data type to pass to tooltip + return ( + alt.Chart(pd.DataFrame({"a": [1]})) + .mark_point() + .encode(tooltip=[{"wrong"}]) + .facet() + ) + + +def chart_error_wrong_tooltip_type_in_layered_chart(): + # Error: Wrong data type to pass to tooltip + return alt.layer( + alt.Chart().mark_point().encode(tooltip=[{"wrong"}]), + ) + + def chart_error_two_errors_in_layered_chart(): # Error 1: Wrong data type to pass to tooltip # Error 2: invalidChannel is not a valid encoding channel @@ -539,12 +556,67 @@ def chart_error_two_errors_in_layered_chart(): def chart_error_two_errors_in_complex_concat_layered_chart(): # Error 1: Wrong data type to pass to tooltip # Error 2: Invalid value for bandPosition - return alt.layer(alt.Chart().mark_point().encode(tooltip=[{"wrong"}])) | ( - alt.Chart(data.cars()) - .mark_text(align="right") - .encode(alt.Text("Horsepower:N", bandPosition="4")) + return ( + chart_error_wrong_tooltip_type_in_layered_chart() + | chart_error_example_invalid_bandposition_value() + ) + + +def chart_error_three_errors_in_complex_concat_layered_chart(): + # Error 1: Wrong data type to pass to tooltip + # Error 2: invalidChannel is not a valid encoding channel + # Error 3: Invalid value for bandPosition + return ( + chart_error_two_errors_in_layered_chart() + | chart_error_example_invalid_bandposition_value() + ) + + +def chart_error_example_two_errors_with_one_in_nested_layered_chart(): + # Error 1: invalidOption is not a valid option for Scale + # Error 2: invalidChannel is not a valid encoding channel + + # In the final chart object, the `layer` attribute will look like this: + # [alt.Chart(...), alt.Chart(...), alt.LayerChart(...)] + # We can therefore use this example to test if an error is also + # spotted in a layered chart within another layered chart + source = pd.DataFrame( + [ + {"Day": 1, "Value": 103.3}, + {"Day": 2, "Value": 394.8}, + {"Day": 3, "Value": 199.5}, + ] ) + blue_bars = ( + alt.Chart(source) + .encode(alt.X("Day:O").scale(invalidOption=10), alt.Y("Value:Q")) + .mark_bar() + ) + red_bars = ( + alt.Chart(source) + .transform_filter(alt.datum.Value >= 300) + .transform_calculate(as_="baseline", calculate="300") + .encode( + alt.X("Day:O"), + alt.Y("baseline:Q"), + alt.Y2("Value:Q"), + color=alt.value("#e45755"), + ) + .mark_bar() + ) + + bars = blue_bars + red_bars + + base = alt.Chart().encode(y=alt.datum(300)) + + rule = base.mark_rule().encode(invalidChannel=2) + text = base.mark_text(text="hazardous") + rule_text = rule + text + + chart = bars + rule_text + return chart + @pytest.mark.parametrize( "chart_func, expected_error_message", @@ -568,6 +640,18 @@ def chart_error_two_errors_in_complex_concat_layered_chart(): - of type 'null' or 'boolean'$""" # noqa: W291 ), ), + ( + chart_error_wrong_tooltip_type_in_faceted_chart, + inspect.cleandoc( + r"""'{'wrong'}' is an invalid value for `field`. Valid values are of type 'string' or 'object'.$""" + ), + ), + ( + chart_error_wrong_tooltip_type_in_layered_chart, + inspect.cleandoc( + r"""'{'wrong'}' is an invalid value for `field`. Valid values are of type 'string' or 'object'.$""" + ), + ), ( chart_error_two_errors_in_layered_chart, inspect.cleandoc( @@ -595,6 +679,55 @@ def chart_error_two_errors_in_complex_concat_layered_chart(): Error \#2: '4' is an invalid value for `bandPosition`. Valid values are of type 'number'.$""" ), ), + ( + chart_error_three_errors_in_complex_concat_layered_chart, + inspect.cleandoc( + r"""Error \#1: '{'wrong'}' is an invalid value for `field`. Valid values are of type 'string' or 'object'. + + Error \#2: `Encoding` has no parameter named 'invalidChannel' + + Existing parameter names are: + angle key order strokeDash tooltip xOffset + color latitude radius strokeOpacity url y + description latitude2 radius2 strokeWidth x y2 + detail longitude shape text x2 yError + fill longitude2 size theta xError yError2 + fillOpacity opacity stroke theta2 xError2 yOffset + href + + See the help for `Encoding` to read the full description of these parameters + + Error \#3: '4' is an invalid value for `bandPosition`. Valid values are of type 'number'.$""" # noqa: W291 + ), + ), + ( + chart_error_example_two_errors_with_one_in_nested_layered_chart, + inspect.cleandoc( + r"""Error \#1: `Scale` has no parameter named 'invalidOption' + + Existing parameter names are: + align domain interpolate range round + base domainMax nice rangeMax scheme + bins domainMid padding rangeMin type + clamp domainMin paddingInner reverse zero + constant exponent paddingOuter + + See the help for `Scale` to read the full description of these parameters + + Error \#2: `Encoding` has no parameter named 'invalidChannel' + + Existing parameter names are: + angle key order strokeDash tooltip xOffset + color latitude radius strokeOpacity url y + description latitude2 radius2 strokeWidth x y2 + detail longitude shape text x2 yError + fill longitude2 size theta xError yError2 + fillOpacity opacity stroke theta2 xError2 yOffset + href + + See the help for `Encoding` to read the full description of these parameters$""" # noqa: W291 + ), + ), ( chart_error_example_layer, inspect.cleandoc( From f28ae9b840e5593c78ae6f25e8bb589f3369126e Mon Sep 17 00:00:00 2001 From: Stefan Binder Date: Sat, 15 Apr 2023 06:56:57 +0000 Subject: [PATCH 26/37] Harmonize chart_error_example function names --- tests/utils/test_schemapi.py | 80 ++++++++++++++++++------------------ 1 file changed, 40 insertions(+), 40 deletions(-) diff --git a/tests/utils/test_schemapi.py b/tests/utils/test_schemapi.py index abf34a236..f4cbaded6 100644 --- a/tests/utils/test_schemapi.py +++ b/tests/utils/test_schemapi.py @@ -393,7 +393,7 @@ def test_schema_validation_error(): assert the_err.message in message -def chart_error_example_layer(): +def chart_error_example__layer(): # Error: Width is not a valid property of a VConcatChart points = ( alt.Chart(data.cars.url) @@ -406,7 +406,7 @@ def chart_error_example_layer(): return (points & points).properties(width=400) -def chart_error_example_hconcat(): +def chart_error_example__hconcat(): # Error: Invalid value for title in Text source = data.cars() points = ( @@ -429,7 +429,7 @@ def chart_error_example_hconcat(): return points | text -def chart_error_example_invalid_channel(): +def chart_error_example__invalid_channel(): # Error: invalidChannel is an invalid encoding channel. Condition is correct # but is added below as in previous implementations of Altair this interfered # with finding the invalidChannel error @@ -445,7 +445,7 @@ def chart_error_example_invalid_channel(): ) -def chart_error_example_invalid_y_option_value_unknown_x_option(): +def chart_error_example__invalid_y_option_value_unknown_x_option(): # Error 1: unknown is an invalid channel option for X # Error 2: Invalid Y option value "asdf" and unknown option "unknown" for X return ( @@ -458,7 +458,7 @@ def chart_error_example_invalid_y_option_value_unknown_x_option(): ) -def chart_error_example_invalid_y_option_value(): +def chart_error_example__invalid_y_option_value(): # Error: Invalid Y option value "asdf" return ( alt.Chart(data.barley()) @@ -470,7 +470,7 @@ def chart_error_example_invalid_y_option_value(): ) -def chart_error_example_invalid_y_option_value_with_condition(): +def chart_error_example__invalid_y_option_value_with_condition(): # Error: Invalid Y option value "asdf". Condition is correct # but is added below as in previous implementations of Altair this interfered # with finding the invalidChannel error @@ -485,17 +485,17 @@ def chart_error_example_invalid_y_option_value_with_condition(): ) -def chart_error_example_invalid_timeunit_value(): +def chart_error_example__invalid_timeunit_value(): # Error: Invalid value for Angle.timeUnit return alt.Chart().encode(alt.Angle().timeUnit("invalid_value")) -def chart_error_example_invalid_sort_value(): +def chart_error_example__invalid_sort_value(): # Error: Invalid value for Angle.sort return alt.Chart().encode(alt.Angle().sort("invalid_value")) -def chart_error_example_invalid_bandposition_value(): +def chart_error_example__invalid_bandposition_value(): # Error: Invalid value for Text.bandPosition return ( alt.Chart(data.cars()) @@ -504,17 +504,17 @@ def chart_error_example_invalid_bandposition_value(): ) -def chart_error_invalid_type(): +def chart_error_example__invalid_type(): # Error: Invalid value for type return alt.Chart().encode(alt.X(type="unknown")) -def chart_error_additional_datum_argument(): +def chart_error_example__additional_datum_argument(): # Error: wrong_argument is not a valid argument to datum return alt.Chart().mark_point().encode(x=alt.datum(1, wrong_argument=1)) -def chart_error_invalid_value_type(): +def chart_error_example__invalid_value_type(): # Error: Value cannot be an integer in this case return ( alt.Chart(data.cars()) @@ -527,7 +527,7 @@ def chart_error_invalid_value_type(): ) -def chart_error_wrong_tooltip_type_in_faceted_chart(): +def chart_error_example__wrong_tooltip_type_in_faceted_chart(): # Error: Wrong data type to pass to tooltip return ( alt.Chart(pd.DataFrame({"a": [1]})) @@ -537,14 +537,14 @@ def chart_error_wrong_tooltip_type_in_faceted_chart(): ) -def chart_error_wrong_tooltip_type_in_layered_chart(): +def chart_error_example__wrong_tooltip_type_in_layered_chart(): # Error: Wrong data type to pass to tooltip return alt.layer( alt.Chart().mark_point().encode(tooltip=[{"wrong"}]), ) -def chart_error_two_errors_in_layered_chart(): +def chart_error_example__two_errors_in_layered_chart(): # Error 1: Wrong data type to pass to tooltip # Error 2: invalidChannel is not a valid encoding channel return alt.layer( @@ -553,26 +553,26 @@ def chart_error_two_errors_in_layered_chart(): ) -def chart_error_two_errors_in_complex_concat_layered_chart(): +def chart_error_example__two_errors_in_complex_concat_layered_chart(): # Error 1: Wrong data type to pass to tooltip # Error 2: Invalid value for bandPosition return ( - chart_error_wrong_tooltip_type_in_layered_chart() - | chart_error_example_invalid_bandposition_value() + chart_error_example__wrong_tooltip_type_in_layered_chart() + | chart_error_example__invalid_bandposition_value() ) -def chart_error_three_errors_in_complex_concat_layered_chart(): +def chart_error_example__three_errors_in_complex_concat_layered_chart(): # Error 1: Wrong data type to pass to tooltip # Error 2: invalidChannel is not a valid encoding channel # Error 3: Invalid value for bandPosition return ( - chart_error_two_errors_in_layered_chart() - | chart_error_example_invalid_bandposition_value() + chart_error_example__two_errors_in_layered_chart() + | chart_error_example__invalid_bandposition_value() ) -def chart_error_example_two_errors_with_one_in_nested_layered_chart(): +def chart_error_example__two_errors_with_one_in_nested_layered_chart(): # Error 1: invalidOption is not a valid option for Scale # Error 2: invalidChannel is not a valid encoding channel @@ -622,7 +622,7 @@ def chart_error_example_two_errors_with_one_in_nested_layered_chart(): "chart_func, expected_error_message", [ ( - chart_error_example_invalid_y_option_value_unknown_x_option, + chart_error_example__invalid_y_option_value_unknown_x_option, inspect.cleandoc( r"""Error \#1: `X` has no parameter named 'unknown' @@ -641,19 +641,19 @@ def chart_error_example_two_errors_with_one_in_nested_layered_chart(): ), ), ( - chart_error_wrong_tooltip_type_in_faceted_chart, + chart_error_example__wrong_tooltip_type_in_faceted_chart, inspect.cleandoc( r"""'{'wrong'}' is an invalid value for `field`. Valid values are of type 'string' or 'object'.$""" ), ), ( - chart_error_wrong_tooltip_type_in_layered_chart, + chart_error_example__wrong_tooltip_type_in_layered_chart, inspect.cleandoc( r"""'{'wrong'}' is an invalid value for `field`. Valid values are of type 'string' or 'object'.$""" ), ), ( - chart_error_two_errors_in_layered_chart, + chart_error_example__two_errors_in_layered_chart, inspect.cleandoc( r"""Error \#1: '{'wrong'}' is an invalid value for `field`. Valid values are of type 'string' or 'object'. @@ -672,7 +672,7 @@ def chart_error_example_two_errors_with_one_in_nested_layered_chart(): ), ), ( - chart_error_two_errors_in_complex_concat_layered_chart, + chart_error_example__two_errors_in_complex_concat_layered_chart, inspect.cleandoc( r"""Error \#1: '{'wrong'}' is an invalid value for `field`. Valid values are of type 'string' or 'object'. @@ -680,7 +680,7 @@ def chart_error_example_two_errors_with_one_in_nested_layered_chart(): ), ), ( - chart_error_three_errors_in_complex_concat_layered_chart, + chart_error_example__three_errors_in_complex_concat_layered_chart, inspect.cleandoc( r"""Error \#1: '{'wrong'}' is an invalid value for `field`. Valid values are of type 'string' or 'object'. @@ -701,7 +701,7 @@ def chart_error_example_two_errors_with_one_in_nested_layered_chart(): ), ), ( - chart_error_example_two_errors_with_one_in_nested_layered_chart, + chart_error_example__two_errors_with_one_in_nested_layered_chart, inspect.cleandoc( r"""Error \#1: `Scale` has no parameter named 'invalidOption' @@ -729,7 +729,7 @@ def chart_error_example_two_errors_with_one_in_nested_layered_chart(): ), ), ( - chart_error_example_layer, + chart_error_example__layer, inspect.cleandoc( r"""`VConcatChart` has no parameter named 'width' @@ -743,7 +743,7 @@ def chart_error_example_two_errors_with_one_in_nested_layered_chart(): ), ), ( - chart_error_example_invalid_y_option_value, + chart_error_example__invalid_y_option_value, inspect.cleandoc( r"""'asdf' is an invalid value for `stack`. Valid values are: @@ -752,7 +752,7 @@ def chart_error_example_two_errors_with_one_in_nested_layered_chart(): ), ), ( - chart_error_example_invalid_y_option_value_with_condition, + chart_error_example__invalid_y_option_value_with_condition, inspect.cleandoc( r"""'asdf' is an invalid value for `stack`. Valid values are: @@ -761,13 +761,13 @@ def chart_error_example_two_errors_with_one_in_nested_layered_chart(): ), ), ( - chart_error_example_hconcat, + chart_error_example__hconcat, inspect.cleandoc( r"""'{'text': 'Horsepower', 'align': 'right'}' is an invalid value for `title`. Valid values are of type 'string', 'array', or 'null'.$""" ), ), ( - chart_error_example_invalid_channel, + chart_error_example__invalid_channel, inspect.cleandoc( r"""`Encoding` has no parameter named 'invalidChannel' @@ -784,7 +784,7 @@ def chart_error_example_two_errors_with_one_in_nested_layered_chart(): ), ), ( - chart_error_example_invalid_timeunit_value, + chart_error_example__invalid_timeunit_value, inspect.cleandoc( r"""'invalid_value' is an invalid value for `timeUnit`. Valid values are: @@ -796,7 +796,7 @@ def chart_error_example_two_errors_with_one_in_nested_layered_chart(): ), ), ( - chart_error_example_invalid_sort_value, + chart_error_example__invalid_sort_value, inspect.cleandoc( r"""'invalid_value' is an invalid value for `sort`. Valid values are: @@ -807,19 +807,19 @@ def chart_error_example_two_errors_with_one_in_nested_layered_chart(): ), ), ( - chart_error_example_invalid_bandposition_value, + chart_error_example__invalid_bandposition_value, inspect.cleandoc( r"""'4' is an invalid value for `bandPosition`. Valid values are of type 'number'.$""" ), ), ( - chart_error_invalid_type, + chart_error_example__invalid_type, inspect.cleandoc( r"""'unknown' is an invalid value for `type`. Valid values are one of \['quantitative', 'ordinal', 'temporal', 'nominal', 'geojson'\].$""" ), ), ( - chart_error_additional_datum_argument, + chart_error_example__additional_datum_argument, inspect.cleandoc( r"""`X` has no parameter named 'wrong_argument' @@ -833,7 +833,7 @@ def chart_error_example_two_errors_with_one_in_nested_layered_chart(): ), ), ( - chart_error_invalid_value_type, + chart_error_example__invalid_value_type, inspect.cleandoc( r"""'1' is an invalid value for `value`. Valid values are of type 'object', 'string', or 'null'.$""" ), From 09805a3a1cb675bcfe26404415632aed7068f9f9 Mon Sep 17 00:00:00 2001 From: Stefan Binder Date: Sat, 15 Apr 2023 07:02:57 +0000 Subject: [PATCH 27/37] Somehow the VL 5.6.1 schema changed slightly. Including the changes here as we upgrade to 5.7 anyway before a release --- altair/vegalite/v5/schema/core.py | 4 ++-- altair/vegalite/v5/schema/vega-lite-schema.json | 16 +++------------- 2 files changed, 5 insertions(+), 15 deletions(-) diff --git a/altair/vegalite/v5/schema/core.py b/altair/vegalite/v5/schema/core.py index 86eb06675..3cd4e23bf 100644 --- a/altair/vegalite/v5/schema/core.py +++ b/altair/vegalite/v5/schema/core.py @@ -16754,7 +16754,7 @@ class LayerRepeatSpec(RepeatSpec): ``"column"`` to the listed fields to be repeated along the particular orientations. The objects ``{"repeat": "row"}`` and ``{"repeat": "column"}`` can be used to refer to the repeated field respectively. - spec : anyOf(:class:`LayerSpec`, :class:`UnitSpec`) + spec : anyOf(:class:`LayerSpec`, :class:`UnitSpecWithFrame`) A specification of the view that gets repeated. align : anyOf(:class:`LayoutAlign`, :class:`RowColLayoutAlign`) The alignment to apply to grid rows and columns. The supported string values are @@ -18978,7 +18978,7 @@ class TopLevelSelectionParameter(TopLevelParameter): **See also:** `init `__ documentation. - views : List(anyOf(string, List(string))) + views : List(string) By default, top-level selections are applied to every view in the visualization. If this property is specified, selections will only be applied to views with the given names. diff --git a/altair/vegalite/v5/schema/vega-lite-schema.json b/altair/vegalite/v5/schema/vega-lite-schema.json index 75e4602c1..855c2b60a 100644 --- a/altair/vegalite/v5/schema/vega-lite-schema.json +++ b/altair/vegalite/v5/schema/vega-lite-schema.json @@ -12467,7 +12467,7 @@ "$ref": "#/definitions/LayerSpec" }, { - "$ref": "#/definitions/UnitSpec" + "$ref": "#/definitions/UnitSpecWithFrame" } ], "description": "A specification of the view that gets repeated." @@ -29223,7 +29223,7 @@ "$ref": "#/definitions/LayerSpec" }, { - "$ref": "#/definitions/UnitSpec" + "$ref": "#/definitions/UnitSpecWithFrame" } ], "description": "A specification of the view that gets repeated." @@ -29507,17 +29507,7 @@ "views": { "description": "By default, top-level selections are applied to every view in the visualization. If this property is specified, selections will only be applied to views with the given names.", "items": { - "anyOf": [ - { - "type": "string" - }, - { - "items": { - "type": "string" - }, - "type": "array" - } - ] + "type": "string" }, "type": "array" } From 994308b7a786858272573a137b788fa3908394cf Mon Sep 17 00:00:00 2001 From: Stefan Binder Date: Sat, 15 Apr 2023 11:29:42 +0000 Subject: [PATCH 28/37] Revert "Somehow the VL 5.6.1 schema changed slightly. Including the changes here as we upgrade to 5.7 anyway before a release" This reverts commit 09805a3a1cb675bcfe26404415632aed7068f9f9. --- altair/vegalite/v5/schema/core.py | 4 ++-- altair/vegalite/v5/schema/vega-lite-schema.json | 16 +++++++++++++--- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/altair/vegalite/v5/schema/core.py b/altair/vegalite/v5/schema/core.py index 3cd4e23bf..86eb06675 100644 --- a/altair/vegalite/v5/schema/core.py +++ b/altair/vegalite/v5/schema/core.py @@ -16754,7 +16754,7 @@ class LayerRepeatSpec(RepeatSpec): ``"column"`` to the listed fields to be repeated along the particular orientations. The objects ``{"repeat": "row"}`` and ``{"repeat": "column"}`` can be used to refer to the repeated field respectively. - spec : anyOf(:class:`LayerSpec`, :class:`UnitSpecWithFrame`) + spec : anyOf(:class:`LayerSpec`, :class:`UnitSpec`) A specification of the view that gets repeated. align : anyOf(:class:`LayoutAlign`, :class:`RowColLayoutAlign`) The alignment to apply to grid rows and columns. The supported string values are @@ -18978,7 +18978,7 @@ class TopLevelSelectionParameter(TopLevelParameter): **See also:** `init `__ documentation. - views : List(string) + views : List(anyOf(string, List(string))) By default, top-level selections are applied to every view in the visualization. If this property is specified, selections will only be applied to views with the given names. diff --git a/altair/vegalite/v5/schema/vega-lite-schema.json b/altair/vegalite/v5/schema/vega-lite-schema.json index 855c2b60a..75e4602c1 100644 --- a/altair/vegalite/v5/schema/vega-lite-schema.json +++ b/altair/vegalite/v5/schema/vega-lite-schema.json @@ -12467,7 +12467,7 @@ "$ref": "#/definitions/LayerSpec" }, { - "$ref": "#/definitions/UnitSpecWithFrame" + "$ref": "#/definitions/UnitSpec" } ], "description": "A specification of the view that gets repeated." @@ -29223,7 +29223,7 @@ "$ref": "#/definitions/LayerSpec" }, { - "$ref": "#/definitions/UnitSpecWithFrame" + "$ref": "#/definitions/UnitSpec" } ], "description": "A specification of the view that gets repeated." @@ -29507,7 +29507,17 @@ "views": { "description": "By default, top-level selections are applied to every view in the visualization. If this property is specified, selections will only be applied to views with the given names.", "items": { - "type": "string" + "anyOf": [ + { + "type": "string" + }, + { + "items": { + "type": "string" + }, + "type": "array" + } + ] }, "type": "array" } From ee484cbb680ca0eaf6d43230ec3cd8a1571e8dc8 Mon Sep 17 00:00:00 2001 From: Stefan Binder Date: Sat, 15 Apr 2023 18:18:23 +0000 Subject: [PATCH 29/37] Remove # in front of error number --- altair/utils/schemapi.py | 2 +- tests/utils/test_schemapi.py | 22 +++++++++++----------- tools/schemapi/schemapi.py | 2 +- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/altair/utils/schemapi.py b/altair/utils/schemapi.py index fe1f62dd3..5fae1d494 100644 --- a/altair/utils/schemapi.py +++ b/altair/utils/schemapi.py @@ -362,7 +362,7 @@ def _get_message(self) -> str: if len(error_messages) > 1: error_messages = [ - f"Error #{error_id}: {message}" + f"Error {error_id}: {message}" for error_id, message in enumerate(error_messages, start=1) ] return ("\n\n").join(error_messages) diff --git a/tests/utils/test_schemapi.py b/tests/utils/test_schemapi.py index f4cbaded6..3756c2324 100644 --- a/tests/utils/test_schemapi.py +++ b/tests/utils/test_schemapi.py @@ -624,7 +624,7 @@ def chart_error_example__two_errors_with_one_in_nested_layered_chart(): ( chart_error_example__invalid_y_option_value_unknown_x_option, inspect.cleandoc( - r"""Error \#1: `X` has no parameter named 'unknown' + r"""Error 1: `X` has no parameter named 'unknown' Existing parameter names are: shorthand bin scale timeUnit @@ -634,7 +634,7 @@ def chart_error_example__two_errors_with_one_in_nested_layered_chart(): See the help for `X` to read the full description of these parameters - Error \#2: 'asdf' is an invalid value for `stack`. Valid values are: + Error 2: 'asdf' is an invalid value for `stack`. Valid values are: - one of \['zero', 'center', 'normalize'\] - of type 'null' or 'boolean'$""" # noqa: W291 @@ -655,9 +655,9 @@ def chart_error_example__two_errors_with_one_in_nested_layered_chart(): ( chart_error_example__two_errors_in_layered_chart, inspect.cleandoc( - r"""Error \#1: '{'wrong'}' is an invalid value for `field`. Valid values are of type 'string' or 'object'. + r"""Error 1: '{'wrong'}' is an invalid value for `field`. Valid values are of type 'string' or 'object'. - Error \#2: `Encoding` has no parameter named 'invalidChannel' + Error 2: `Encoding` has no parameter named 'invalidChannel' Existing parameter names are: angle key order strokeDash tooltip xOffset @@ -674,17 +674,17 @@ def chart_error_example__two_errors_with_one_in_nested_layered_chart(): ( chart_error_example__two_errors_in_complex_concat_layered_chart, inspect.cleandoc( - r"""Error \#1: '{'wrong'}' is an invalid value for `field`. Valid values are of type 'string' or 'object'. + r"""Error 1: '{'wrong'}' is an invalid value for `field`. Valid values are of type 'string' or 'object'. - Error \#2: '4' is an invalid value for `bandPosition`. Valid values are of type 'number'.$""" + Error 2: '4' is an invalid value for `bandPosition`. Valid values are of type 'number'.$""" ), ), ( chart_error_example__three_errors_in_complex_concat_layered_chart, inspect.cleandoc( - r"""Error \#1: '{'wrong'}' is an invalid value for `field`. Valid values are of type 'string' or 'object'. + r"""Error 1: '{'wrong'}' is an invalid value for `field`. Valid values are of type 'string' or 'object'. - Error \#2: `Encoding` has no parameter named 'invalidChannel' + Error 2: `Encoding` has no parameter named 'invalidChannel' Existing parameter names are: angle key order strokeDash tooltip xOffset @@ -697,13 +697,13 @@ def chart_error_example__two_errors_with_one_in_nested_layered_chart(): See the help for `Encoding` to read the full description of these parameters - Error \#3: '4' is an invalid value for `bandPosition`. Valid values are of type 'number'.$""" # noqa: W291 + Error 3: '4' is an invalid value for `bandPosition`. Valid values are of type 'number'.$""" # noqa: W291 ), ), ( chart_error_example__two_errors_with_one_in_nested_layered_chart, inspect.cleandoc( - r"""Error \#1: `Scale` has no parameter named 'invalidOption' + r"""Error 1: `Scale` has no parameter named 'invalidOption' Existing parameter names are: align domain interpolate range round @@ -714,7 +714,7 @@ def chart_error_example__two_errors_with_one_in_nested_layered_chart(): See the help for `Scale` to read the full description of these parameters - Error \#2: `Encoding` has no parameter named 'invalidChannel' + Error 2: `Encoding` has no parameter named 'invalidChannel' Existing parameter names are: angle key order strokeDash tooltip xOffset diff --git a/tools/schemapi/schemapi.py b/tools/schemapi/schemapi.py index 983f92a6c..8c9ab840e 100644 --- a/tools/schemapi/schemapi.py +++ b/tools/schemapi/schemapi.py @@ -360,7 +360,7 @@ def _get_message(self) -> str: if len(error_messages) > 1: error_messages = [ - f"Error #{error_id}: {message}" + f"Error {error_id}: {message}" for error_id, message in enumerate(error_messages, start=1) ] return ("\n\n").join(error_messages) From 2b95b38919956dac24fd3b7a4f5dcfe81fbbbb21 Mon Sep 17 00:00:00 2001 From: Stefan Binder Date: Sat, 15 Apr 2023 18:22:12 +0000 Subject: [PATCH 30/37] Add 'Multiple errors were found.' as a first line if multiple errors are found --- altair/utils/schemapi.py | 5 ++++- tests/utils/test_schemapi.py | 20 +++++++++++++++----- tools/schemapi/schemapi.py | 5 ++++- 3 files changed, 23 insertions(+), 7 deletions(-) diff --git a/altair/utils/schemapi.py b/altair/utils/schemapi.py index 5fae1d494..490d65970 100644 --- a/altair/utils/schemapi.py +++ b/altair/utils/schemapi.py @@ -360,12 +360,15 @@ def _get_message(self) -> str: for errors in self._errors.values(): error_messages.append(self._get_message_for_errors_group(errors)) + message = "" if len(error_messages) > 1: error_messages = [ f"Error {error_id}: {message}" for error_id, message in enumerate(error_messages, start=1) ] - return ("\n\n").join(error_messages) + message += "Multiple errors were found.\n\n" + message += "\n\n".join(error_messages) + return message def _get_message_for_errors_group( self, diff --git a/tests/utils/test_schemapi.py b/tests/utils/test_schemapi.py index 3756c2324..92c21e534 100644 --- a/tests/utils/test_schemapi.py +++ b/tests/utils/test_schemapi.py @@ -624,7 +624,9 @@ def chart_error_example__two_errors_with_one_in_nested_layered_chart(): ( chart_error_example__invalid_y_option_value_unknown_x_option, inspect.cleandoc( - r"""Error 1: `X` has no parameter named 'unknown' + r"""Multiple errors were found. + + Error 1: `X` has no parameter named 'unknown' Existing parameter names are: shorthand bin scale timeUnit @@ -655,7 +657,9 @@ def chart_error_example__two_errors_with_one_in_nested_layered_chart(): ( chart_error_example__two_errors_in_layered_chart, inspect.cleandoc( - r"""Error 1: '{'wrong'}' is an invalid value for `field`. Valid values are of type 'string' or 'object'. + r"""Multiple errors were found. + + Error 1: '{'wrong'}' is an invalid value for `field`. Valid values are of type 'string' or 'object'. Error 2: `Encoding` has no parameter named 'invalidChannel' @@ -674,7 +678,9 @@ def chart_error_example__two_errors_with_one_in_nested_layered_chart(): ( chart_error_example__two_errors_in_complex_concat_layered_chart, inspect.cleandoc( - r"""Error 1: '{'wrong'}' is an invalid value for `field`. Valid values are of type 'string' or 'object'. + r"""Multiple errors were found. + + Error 1: '{'wrong'}' is an invalid value for `field`. Valid values are of type 'string' or 'object'. Error 2: '4' is an invalid value for `bandPosition`. Valid values are of type 'number'.$""" ), @@ -682,7 +688,9 @@ def chart_error_example__two_errors_with_one_in_nested_layered_chart(): ( chart_error_example__three_errors_in_complex_concat_layered_chart, inspect.cleandoc( - r"""Error 1: '{'wrong'}' is an invalid value for `field`. Valid values are of type 'string' or 'object'. + r"""Multiple errors were found. + + Error 1: '{'wrong'}' is an invalid value for `field`. Valid values are of type 'string' or 'object'. Error 2: `Encoding` has no parameter named 'invalidChannel' @@ -703,7 +711,9 @@ def chart_error_example__two_errors_with_one_in_nested_layered_chart(): ( chart_error_example__two_errors_with_one_in_nested_layered_chart, inspect.cleandoc( - r"""Error 1: `Scale` has no parameter named 'invalidOption' + r"""Multiple errors were found. + + Error 1: `Scale` has no parameter named 'invalidOption' Existing parameter names are: align domain interpolate range round diff --git a/tools/schemapi/schemapi.py b/tools/schemapi/schemapi.py index 8c9ab840e..5dd802740 100644 --- a/tools/schemapi/schemapi.py +++ b/tools/schemapi/schemapi.py @@ -358,12 +358,15 @@ def _get_message(self) -> str: for errors in self._errors.values(): error_messages.append(self._get_message_for_errors_group(errors)) + message = "" if len(error_messages) > 1: error_messages = [ f"Error {error_id}: {message}" for error_id, message in enumerate(error_messages, start=1) ] - return ("\n\n").join(error_messages) + message += "Multiple errors were found.\n\n" + message += "\n\n".join(error_messages) + return message def _get_message_for_errors_group( self, From 62e6b1a514d9e82c485ca69245326f0ee4864d80 Mon Sep 17 00:00:00 2001 From: Stefan Binder Date: Sat, 15 Apr 2023 18:33:09 +0000 Subject: [PATCH 31/37] Indent error messages from the second line onwards if multiple errors are shown --- altair/utils/schemapi.py | 12 +++++- tests/utils/test_schemapi.py | 84 ++++++++++++++++++------------------ tools/schemapi/schemapi.py | 12 +++++- 3 files changed, 62 insertions(+), 46 deletions(-) diff --git a/altair/utils/schemapi.py b/altair/utils/schemapi.py index 490d65970..022b2e840 100644 --- a/altair/utils/schemapi.py +++ b/altair/utils/schemapi.py @@ -356,6 +356,14 @@ def __str__(self) -> str: return self.message def _get_message(self) -> str: + def indent_second_line_onwards(message: str, indent: int = 4) -> str: + modified_lines: List[str] = [] + for idx, line in enumerate(message.split("\n")): + if idx > 0 and len(line) > 0: + line = " " * indent + line + modified_lines.append(line) + return "\n".join(modified_lines) + error_messages: List[str] = [] for errors in self._errors.values(): error_messages.append(self._get_message_for_errors_group(errors)) @@ -363,8 +371,8 @@ def _get_message(self) -> str: message = "" if len(error_messages) > 1: error_messages = [ - f"Error {error_id}: {message}" - for error_id, message in enumerate(error_messages, start=1) + indent_second_line_onwards(f"Error {error_id}: {m}") + for error_id, m in enumerate(error_messages, start=1) ] message += "Multiple errors were found.\n\n" message += "\n\n".join(error_messages) diff --git a/tests/utils/test_schemapi.py b/tests/utils/test_schemapi.py index 92c21e534..a4ef2c150 100644 --- a/tests/utils/test_schemapi.py +++ b/tests/utils/test_schemapi.py @@ -628,18 +628,18 @@ def chart_error_example__two_errors_with_one_in_nested_layered_chart(): Error 1: `X` has no parameter named 'unknown' - Existing parameter names are: - shorthand bin scale timeUnit - aggregate field sort title - axis impute stack type - bandPosition + Existing parameter names are: + shorthand bin scale timeUnit + aggregate field sort title + axis impute stack type + bandPosition - See the help for `X` to read the full description of these parameters + See the help for `X` to read the full description of these parameters Error 2: 'asdf' is an invalid value for `stack`. Valid values are: - - one of \['zero', 'center', 'normalize'\] - - of type 'null' or 'boolean'$""" # noqa: W291 + - one of \['zero', 'center', 'normalize'\] + - of type 'null' or 'boolean'$""" # noqa: W291 ), ), ( @@ -663,16 +663,16 @@ def chart_error_example__two_errors_with_one_in_nested_layered_chart(): Error 2: `Encoding` has no parameter named 'invalidChannel' - Existing parameter names are: - angle key order strokeDash tooltip xOffset - color latitude radius strokeOpacity url y - description latitude2 radius2 strokeWidth x y2 - detail longitude shape text x2 yError - fill longitude2 size theta xError yError2 - fillOpacity opacity stroke theta2 xError2 yOffset - href + Existing parameter names are: + angle key order strokeDash tooltip xOffset + color latitude radius strokeOpacity url y + description latitude2 radius2 strokeWidth x y2 + detail longitude shape text x2 yError + fill longitude2 size theta xError yError2 + fillOpacity opacity stroke theta2 xError2 yOffset + href - See the help for `Encoding` to read the full description of these parameters$""" # noqa: W291 + See the help for `Encoding` to read the full description of these parameters$""" # noqa: W291 ), ), ( @@ -694,16 +694,16 @@ def chart_error_example__two_errors_with_one_in_nested_layered_chart(): Error 2: `Encoding` has no parameter named 'invalidChannel' - Existing parameter names are: - angle key order strokeDash tooltip xOffset - color latitude radius strokeOpacity url y - description latitude2 radius2 strokeWidth x y2 - detail longitude shape text x2 yError - fill longitude2 size theta xError yError2 - fillOpacity opacity stroke theta2 xError2 yOffset - href + Existing parameter names are: + angle key order strokeDash tooltip xOffset + color latitude radius strokeOpacity url y + description latitude2 radius2 strokeWidth x y2 + detail longitude shape text x2 yError + fill longitude2 size theta xError yError2 + fillOpacity opacity stroke theta2 xError2 yOffset + href - See the help for `Encoding` to read the full description of these parameters + See the help for `Encoding` to read the full description of these parameters Error 3: '4' is an invalid value for `bandPosition`. Valid values are of type 'number'.$""" # noqa: W291 ), @@ -715,27 +715,27 @@ def chart_error_example__two_errors_with_one_in_nested_layered_chart(): Error 1: `Scale` has no parameter named 'invalidOption' - Existing parameter names are: - align domain interpolate range round - base domainMax nice rangeMax scheme - bins domainMid padding rangeMin type - clamp domainMin paddingInner reverse zero - constant exponent paddingOuter + Existing parameter names are: + align domain interpolate range round + base domainMax nice rangeMax scheme + bins domainMid padding rangeMin type + clamp domainMin paddingInner reverse zero + constant exponent paddingOuter - See the help for `Scale` to read the full description of these parameters + See the help for `Scale` to read the full description of these parameters Error 2: `Encoding` has no parameter named 'invalidChannel' - Existing parameter names are: - angle key order strokeDash tooltip xOffset - color latitude radius strokeOpacity url y - description latitude2 radius2 strokeWidth x y2 - detail longitude shape text x2 yError - fill longitude2 size theta xError yError2 - fillOpacity opacity stroke theta2 xError2 yOffset - href + Existing parameter names are: + angle key order strokeDash tooltip xOffset + color latitude radius strokeOpacity url y + description latitude2 radius2 strokeWidth x y2 + detail longitude shape text x2 yError + fill longitude2 size theta xError yError2 + fillOpacity opacity stroke theta2 xError2 yOffset + href - See the help for `Encoding` to read the full description of these parameters$""" # noqa: W291 + See the help for `Encoding` to read the full description of these parameters$""" # noqa: W291 ), ), ( diff --git a/tools/schemapi/schemapi.py b/tools/schemapi/schemapi.py index 5dd802740..5d167f814 100644 --- a/tools/schemapi/schemapi.py +++ b/tools/schemapi/schemapi.py @@ -354,6 +354,14 @@ def __str__(self) -> str: return self.message def _get_message(self) -> str: + def indent_second_line_onwards(message: str, indent: int = 4) -> str: + modified_lines: List[str] = [] + for idx, line in enumerate(message.split("\n")): + if idx > 0 and len(line) > 0: + line = " " * indent + line + modified_lines.append(line) + return "\n".join(modified_lines) + error_messages: List[str] = [] for errors in self._errors.values(): error_messages.append(self._get_message_for_errors_group(errors)) @@ -361,8 +369,8 @@ def _get_message(self) -> str: message = "" if len(error_messages) > 1: error_messages = [ - f"Error {error_id}: {message}" - for error_id, message in enumerate(error_messages, start=1) + indent_second_line_onwards(f"Error {error_id}: {m}") + for error_id, m in enumerate(error_messages, start=1) ] message += "Multiple errors were found.\n\n" message += "\n\n".join(error_messages) From 166b4f4aed6b18b0684a8f1ec845268f96595e79 Mon Sep 17 00:00:00 2001 From: Stefan Binder Date: Sat, 15 Apr 2023 18:43:38 +0000 Subject: [PATCH 32/37] Show a maximum of 3 errors --- altair/utils/schemapi.py | 3 +- tests/utils/test_schemapi.py | 53 ++++++++++++++++++++++++++++++++++++ tools/schemapi/schemapi.py | 3 +- 3 files changed, 57 insertions(+), 2 deletions(-) diff --git a/altair/utils/schemapi.py b/altair/utils/schemapi.py index 022b2e840..3d8525aad 100644 --- a/altair/utils/schemapi.py +++ b/altair/utils/schemapi.py @@ -365,7 +365,8 @@ def indent_second_line_onwards(message: str, indent: int = 4) -> str: return "\n".join(modified_lines) error_messages: List[str] = [] - for errors in self._errors.values(): + # Only show a maximum of 3 errors as else the messages could get very long. + for errors in list(self._errors.values())[:3]: error_messages.append(self._get_message_for_errors_group(errors)) message = "" diff --git a/tests/utils/test_schemapi.py b/tests/utils/test_schemapi.py index a4ef2c150..606242f45 100644 --- a/tests/utils/test_schemapi.py +++ b/tests/utils/test_schemapi.py @@ -618,6 +618,25 @@ def chart_error_example__two_errors_with_one_in_nested_layered_chart(): return chart +def chart_error_example__four_errors(): + # Error 1: unknown is not a valid encoding channel option + # Error 2: Invalid Y option value "asdf". + # Error 3: another_unknown is not a valid encoding channel option + # Error 4: fourth_error is not a valid encoding channel option <- this error + # should not show up in the final error message as it is capped at showing + # 3 errors + return ( + alt.Chart(data.barley()) + .mark_bar() + .encode( + x=alt.X("variety", unknown=2), + y=alt.Y("sum(yield)", stack="asdf"), + color=alt.Color("variety", another_unknown=2), + opacity=alt.Opacity("variety", fourth_error=1), + ) + ) + + @pytest.mark.parametrize( "chart_func, expected_error_message", [ @@ -848,6 +867,40 @@ def chart_error_example__two_errors_with_one_in_nested_layered_chart(): r"""'1' is an invalid value for `value`. Valid values are of type 'object', 'string', or 'null'.$""" ), ), + ( + chart_error_example__four_errors, + inspect.cleandoc( + r"""Multiple errors were found. + + Error 1: `Color` has no parameter named 'another_unknown' + + Existing parameter names are: + shorthand bin legend timeUnit + aggregate condition scale title + bandPosition field sort type + + See the help for `Color` to read the full description of these parameters + + Error 2: `Opacity` has no parameter named 'fourth_error' + + Existing parameter names are: + shorthand bin legend timeUnit + aggregate condition scale title + bandPosition field sort type + + See the help for `Opacity` to read the full description of these parameters + + Error 3: `X` has no parameter named 'unknown' + + Existing parameter names are: + shorthand bin scale timeUnit + aggregate field sort title + axis impute stack type + bandPosition + + See the help for `X` to read the full description of these parameters$""" # noqa: W291 + ), + ), ], ) def test_chart_validation_errors(chart_func, expected_error_message): diff --git a/tools/schemapi/schemapi.py b/tools/schemapi/schemapi.py index 5d167f814..624e6d583 100644 --- a/tools/schemapi/schemapi.py +++ b/tools/schemapi/schemapi.py @@ -363,7 +363,8 @@ def indent_second_line_onwards(message: str, indent: int = 4) -> str: return "\n".join(modified_lines) error_messages: List[str] = [] - for errors in self._errors.values(): + # Only show a maximum of 3 errors as else the messages could get very long. + for errors in list(self._errors.values())[:3]: error_messages.append(self._get_message_for_errors_group(errors)) message = "" From 2f84e7f2dbf657d91528387c0147e3e7d9db492c Mon Sep 17 00:00:00 2001 From: Stefan Binder Date: Sat, 15 Apr 2023 18:49:08 +0000 Subject: [PATCH 33/37] Some white spaces were removed in a description in the VL 5.6.1 schema. Changes are not relevant as we upgrade to 5.7 anyway. They happened as part of resolving vega/schema #8 --- altair/vegalite/v5/schema/core.py | 6 +++--- altair/vegalite/v5/schema/vega-lite-schema.json | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/altair/vegalite/v5/schema/core.py b/altair/vegalite/v5/schema/core.py index 86eb06675..683fcd0a8 100644 --- a/altair/vegalite/v5/schema/core.py +++ b/altair/vegalite/v5/schema/core.py @@ -14318,9 +14318,9 @@ class Scale(VegaLiteSchema): For * `continuous `__ * scales, expands the scale domain to accommodate the specified number of pixels on each of the scale range. The scale range must represent pixels for this parameter to - function as intended. Padding adjustment is performed prior to all other - adjustments, including the effects of the  ``zero``,  ``nice``,  ``domainMin``, and - ``domainMax``  properties. + function as intended. Padding adjustment is performed prior to all other + adjustments, including the effects of the ``zero``, ``nice``, ``domainMin``, and + ``domainMax`` properties. For * `band `__ * scales, shortcut for setting ``paddingInner`` and ``paddingOuter`` to the same value. diff --git a/altair/vegalite/v5/schema/vega-lite-schema.json b/altair/vegalite/v5/schema/vega-lite-schema.json index 75e4602c1..e234533f6 100644 --- a/altair/vegalite/v5/schema/vega-lite-schema.json +++ b/altair/vegalite/v5/schema/vega-lite-schema.json @@ -21215,7 +21215,7 @@ "$ref": "#/definitions/ExprRef" } ], - "description": "For _[continuous](https://vega.github.io/vega-lite/docs/scale.html#continuous)_ scales, expands the scale domain to accommodate the specified number of pixels on each of the scale range. The scale range must represent pixels for this parameter to function as intended. Padding adjustment is performed prior to all other adjustments, including the effects of the `zero`, `nice`, `domainMin`, and `domainMax` properties.\n\nFor _[band](https://vega.github.io/vega-lite/docs/scale.html#band)_ scales, shortcut for setting `paddingInner` and `paddingOuter` to the same value.\n\nFor _[point](https://vega.github.io/vega-lite/docs/scale.html#point)_ scales, alias for `paddingOuter`.\n\n__Default value:__ For _continuous_ scales, derived from the [scale config](https://vega.github.io/vega-lite/docs/scale.html#config)'s `continuousPadding`. For _band and point_ scales, see `paddingInner` and `paddingOuter`. By default, Vega-Lite sets padding such that _width/height = number of unique values * step_.", + "description": "For _[continuous](https://vega.github.io/vega-lite/docs/scale.html#continuous)_ scales, expands the scale domain to accommodate the specified number of pixels on each of the scale range. The scale range must represent pixels for this parameter to function as intended. Padding adjustment is performed prior to all other adjustments, including the effects of the `zero`, `nice`, `domainMin`, and `domainMax` properties.\n\nFor _[band](https://vega.github.io/vega-lite/docs/scale.html#band)_ scales, shortcut for setting `paddingInner` and `paddingOuter` to the same value.\n\nFor _[point](https://vega.github.io/vega-lite/docs/scale.html#point)_ scales, alias for `paddingOuter`.\n\n__Default value:__ For _continuous_ scales, derived from the [scale config](https://vega.github.io/vega-lite/docs/scale.html#config)'s `continuousPadding`. For _band and point_ scales, see `paddingInner` and `paddingOuter`. By default, Vega-Lite sets padding such that _width/height = number of unique values * step_.", "minimum": 0 }, "paddingInner": { @@ -30953,4 +30953,4 @@ "type": "object" } } -} +} \ No newline at end of file From 739922dd0e0387603068e5a114fbe3df6610c877 Mon Sep 17 00:00:00 2001 From: Stefan Binder Date: Mon, 17 Apr 2023 16:43:47 +0000 Subject: [PATCH 34/37] Minor improvements --- altair/utils/schemapi.py | 17 ++++++++++------- tools/schemapi/schemapi.py | 17 ++++++++++------- 2 files changed, 20 insertions(+), 14 deletions(-) diff --git a/altair/utils/schemapi.py b/altair/utils/schemapi.py index 3d8525aad..147945f57 100644 --- a/altair/utils/schemapi.py +++ b/altair/utils/schemapi.py @@ -102,7 +102,7 @@ def _get_errors_from_spec( """Uses the relevant jsonschema validator to validate the passed in spec against the schema using the rootschema to resolve references. The schema and rootschema themselves are not validated but instead considered - as validate. + as valid. """ # We don't use jsonschema.validate as this would validate the schema itself. # Instead, we pass the schema directly to the validator class. This is done for @@ -239,9 +239,10 @@ def _group_errors_by_validator(errors: ValidationErrorList) -> GroupedValidation str, ValidationErrorList ] = collections.defaultdict(list) for err in errors: - # Ignore mypy error as err.validator as it wrongly sees err.validator - # as of type Optional[Validator] instead of str - errors_by_validator[err.validator].append(err) # type: ignore[index] + # Unclear when err.validator could ever be None but as it's the type hint + # returned by the jsonschema package we still guard against this case below + validator = err.validator if err.validator is not None else "Unknown validator" + errors_by_validator[validator].append(err) return dict(errors_by_validator) @@ -280,14 +281,15 @@ def _deduplicate_additional_properties_errors( - "Additional properties are not allowed ('field', 'type', 'unknown' were unexpected)" """ if len(errors) > 1: - # The code below subsets this to only the first error of the three. - parent = errors[0].parent # Test if all parent errors are the same anyOf error and only do # the prioritization in these cases. Can't think of a chart spec where this # would not be the case but still allow for it below to not break anything. + parent = errors[0].parent if ( parent is not None and parent.validator == "anyOf" + # Use [1:] as don't have to check for first error as it was used + # above to define `parent` and all(err.parent is parent for err in errors[1:]) ): errors = [min(errors, key=lambda x: len(x.message))] @@ -365,7 +367,8 @@ def indent_second_line_onwards(message: str, indent: int = 4) -> str: return "\n".join(modified_lines) error_messages: List[str] = [] - # Only show a maximum of 3 errors as else the messages could get very long. + # Only show a maximum of 3 errors as else the final message returned by this + # method could get very long. for errors in list(self._errors.values())[:3]: error_messages.append(self._get_message_for_errors_group(errors)) diff --git a/tools/schemapi/schemapi.py b/tools/schemapi/schemapi.py index 624e6d583..6988e4f2d 100644 --- a/tools/schemapi/schemapi.py +++ b/tools/schemapi/schemapi.py @@ -100,7 +100,7 @@ def _get_errors_from_spec( """Uses the relevant jsonschema validator to validate the passed in spec against the schema using the rootschema to resolve references. The schema and rootschema themselves are not validated but instead considered - as validate. + as valid. """ # We don't use jsonschema.validate as this would validate the schema itself. # Instead, we pass the schema directly to the validator class. This is done for @@ -237,9 +237,10 @@ def _group_errors_by_validator(errors: ValidationErrorList) -> GroupedValidation str, ValidationErrorList ] = collections.defaultdict(list) for err in errors: - # Ignore mypy error as err.validator as it wrongly sees err.validator - # as of type Optional[Validator] instead of str - errors_by_validator[err.validator].append(err) # type: ignore[index] + # Unclear when err.validator could ever be None but as it's the type hint + # returned by the jsonschema package we still guard against this case below + validator = err.validator if err.validator is not None else "Unknown validator" + errors_by_validator[validator].append(err) return dict(errors_by_validator) @@ -278,14 +279,15 @@ def _deduplicate_additional_properties_errors( - "Additional properties are not allowed ('field', 'type', 'unknown' were unexpected)" """ if len(errors) > 1: - # The code below subsets this to only the first error of the three. - parent = errors[0].parent # Test if all parent errors are the same anyOf error and only do # the prioritization in these cases. Can't think of a chart spec where this # would not be the case but still allow for it below to not break anything. + parent = errors[0].parent if ( parent is not None and parent.validator == "anyOf" + # Use [1:] as don't have to check for first error as it was used + # above to define `parent` and all(err.parent is parent for err in errors[1:]) ): errors = [min(errors, key=lambda x: len(x.message))] @@ -363,7 +365,8 @@ def indent_second_line_onwards(message: str, indent: int = 4) -> str: return "\n".join(modified_lines) error_messages: List[str] = [] - # Only show a maximum of 3 errors as else the messages could get very long. + # Only show a maximum of 3 errors as else the final message returned by this + # method could get very long. for errors in list(self._errors.values())[:3]: error_messages.append(self._get_message_for_errors_group(errors)) From fa036eb6c715f8760bec990061a6979968639846 Mon Sep 17 00:00:00 2001 From: Stefan Binder Date: Mon, 17 Apr 2023 18:46:22 +0000 Subject: [PATCH 35/37] Revert minor change --- altair/utils/schemapi.py | 8 ++++---- tools/schemapi/schemapi.py | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/altair/utils/schemapi.py b/altair/utils/schemapi.py index 147945f57..8000fee15 100644 --- a/altair/utils/schemapi.py +++ b/altair/utils/schemapi.py @@ -239,10 +239,10 @@ def _group_errors_by_validator(errors: ValidationErrorList) -> GroupedValidation str, ValidationErrorList ] = collections.defaultdict(list) for err in errors: - # Unclear when err.validator could ever be None but as it's the type hint - # returned by the jsonschema package we still guard against this case below - validator = err.validator if err.validator is not None else "Unknown validator" - errors_by_validator[validator].append(err) + # Ignore mypy error as err.validator as it wrongly sees err.validator + # as of type Optional[Validator] instead of str which it is according + # to the documentation and all tested cases + errors_by_validator[err.validator].append(err) # type: ignore[index] return dict(errors_by_validator) diff --git a/tools/schemapi/schemapi.py b/tools/schemapi/schemapi.py index 6988e4f2d..599f4aaa3 100644 --- a/tools/schemapi/schemapi.py +++ b/tools/schemapi/schemapi.py @@ -237,10 +237,10 @@ def _group_errors_by_validator(errors: ValidationErrorList) -> GroupedValidation str, ValidationErrorList ] = collections.defaultdict(list) for err in errors: - # Unclear when err.validator could ever be None but as it's the type hint - # returned by the jsonschema package we still guard against this case below - validator = err.validator if err.validator is not None else "Unknown validator" - errors_by_validator[validator].append(err) + # Ignore mypy error as err.validator as it wrongly sees err.validator + # as of type Optional[Validator] instead of str which it is according + # to the documentation and all tested cases + errors_by_validator[err.validator].append(err) # type: ignore[index] return dict(errors_by_validator) From e3a96fa6ecd1887f19a41609c5f3775e53c1a2b4 Mon Sep 17 00:00:00 2001 From: mattijn Date: Fri, 21 Apr 2023 11:37:05 +0200 Subject: [PATCH 36/37] re-run generate_schema_wrapper.py --- altair/vegalite/v5/schema/core.py | 6 +++--- altair/vegalite/v5/schema/vega-lite-schema.json | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/altair/vegalite/v5/schema/core.py b/altair/vegalite/v5/schema/core.py index ca6ead186..3cd4e23bf 100644 --- a/altair/vegalite/v5/schema/core.py +++ b/altair/vegalite/v5/schema/core.py @@ -14318,9 +14318,9 @@ class Scale(VegaLiteSchema): For * `continuous `__ * scales, expands the scale domain to accommodate the specified number of pixels on each of the scale range. The scale range must represent pixels for this parameter to - function as intended. Padding adjustment is performed prior to all other - adjustments, including the effects of the ``zero``, ``nice``, ``domainMin``, and - ``domainMax`` properties. + function as intended. Padding adjustment is performed prior to all other + adjustments, including the effects of the  ``zero``,  ``nice``,  ``domainMin``, and + ``domainMax``  properties. For * `band `__ * scales, shortcut for setting ``paddingInner`` and ``paddingOuter`` to the same value. diff --git a/altair/vegalite/v5/schema/vega-lite-schema.json b/altair/vegalite/v5/schema/vega-lite-schema.json index e1f1093c6..855c2b60a 100644 --- a/altair/vegalite/v5/schema/vega-lite-schema.json +++ b/altair/vegalite/v5/schema/vega-lite-schema.json @@ -21215,7 +21215,7 @@ "$ref": "#/definitions/ExprRef" } ], - "description": "For _[continuous](https://vega.github.io/vega-lite/docs/scale.html#continuous)_ scales, expands the scale domain to accommodate the specified number of pixels on each of the scale range. The scale range must represent pixels for this parameter to function as intended. Padding adjustment is performed prior to all other adjustments, including the effects of the `zero`, `nice`, `domainMin`, and `domainMax` properties.\n\nFor _[band](https://vega.github.io/vega-lite/docs/scale.html#band)_ scales, shortcut for setting `paddingInner` and `paddingOuter` to the same value.\n\nFor _[point](https://vega.github.io/vega-lite/docs/scale.html#point)_ scales, alias for `paddingOuter`.\n\n__Default value:__ For _continuous_ scales, derived from the [scale config](https://vega.github.io/vega-lite/docs/scale.html#config)'s `continuousPadding`. For _band and point_ scales, see `paddingInner` and `paddingOuter`. By default, Vega-Lite sets padding such that _width/height = number of unique values * step_.", + "description": "For _[continuous](https://vega.github.io/vega-lite/docs/scale.html#continuous)_ scales, expands the scale domain to accommodate the specified number of pixels on each of the scale range. The scale range must represent pixels for this parameter to function as intended. Padding adjustment is performed prior to all other adjustments, including the effects of the `zero`, `nice`, `domainMin`, and `domainMax` properties.\n\nFor _[band](https://vega.github.io/vega-lite/docs/scale.html#band)_ scales, shortcut for setting `paddingInner` and `paddingOuter` to the same value.\n\nFor _[point](https://vega.github.io/vega-lite/docs/scale.html#point)_ scales, alias for `paddingOuter`.\n\n__Default value:__ For _continuous_ scales, derived from the [scale config](https://vega.github.io/vega-lite/docs/scale.html#config)'s `continuousPadding`. For _band and point_ scales, see `paddingInner` and `paddingOuter`. By default, Vega-Lite sets padding such that _width/height = number of unique values * step_.", "minimum": 0 }, "paddingInner": { @@ -30943,4 +30943,4 @@ "type": "object" } } -} \ No newline at end of file +} From 38e814813ae33911a6564b336a8c5d2e655f96a5 Mon Sep 17 00:00:00 2001 From: Stefan Binder Date: Tue, 25 Apr 2023 06:10:36 +0000 Subject: [PATCH 37/37] Capitalize bullet points --- altair/utils/schemapi.py | 3 +++ tests/utils/test_schemapi.py | 30 +++++++++++++++--------------- tools/schemapi/schemapi.py | 3 +++ 3 files changed, 21 insertions(+), 15 deletions(-) diff --git a/altair/utils/schemapi.py b/altair/utils/schemapi.py index 8000fee15..79808e240 100644 --- a/altair/utils/schemapi.py +++ b/altair/utils/schemapi.py @@ -534,6 +534,9 @@ def _get_default_error_message( elif len(bullet_points) == 1: message += f". Valid values are {bullet_points[0]}.\n\n" else: + # We don't use .capitalize below to make the first letter uppercase + # as that makes the rest of the message lowercase + bullet_points = [point[0].upper() + point[1:] for point in bullet_points] message += ". Valid values are:\n\n" message += "\n".join([f"- {point}" for point in bullet_points]) message += "\n\n" diff --git a/tests/utils/test_schemapi.py b/tests/utils/test_schemapi.py index 606242f45..3e7a6e1f7 100644 --- a/tests/utils/test_schemapi.py +++ b/tests/utils/test_schemapi.py @@ -657,8 +657,8 @@ def chart_error_example__four_errors(): Error 2: 'asdf' is an invalid value for `stack`. Valid values are: - - one of \['zero', 'center', 'normalize'\] - - of type 'null' or 'boolean'$""" # noqa: W291 + - One of \['zero', 'center', 'normalize'\] + - Of type 'null' or 'boolean'$""" # noqa: W291 ), ), ( @@ -776,8 +776,8 @@ def chart_error_example__four_errors(): inspect.cleandoc( r"""'asdf' is an invalid value for `stack`. Valid values are: - - one of \['zero', 'center', 'normalize'\] - - of type 'null' or 'boolean'$""" + - One of \['zero', 'center', 'normalize'\] + - Of type 'null' or 'boolean'$""" ), ), ( @@ -785,8 +785,8 @@ def chart_error_example__four_errors(): inspect.cleandoc( r"""'asdf' is an invalid value for `stack`. Valid values are: - - one of \['zero', 'center', 'normalize'\] - - of type 'null' or 'boolean'$""" + - One of \['zero', 'center', 'normalize'\] + - Of type 'null' or 'boolean'$""" ), ), ( @@ -817,11 +817,11 @@ def chart_error_example__four_errors(): inspect.cleandoc( r"""'invalid_value' is an invalid value for `timeUnit`. Valid values are: - - one of \['year', 'quarter', 'month', 'week', 'day', 'dayofyear', 'date', 'hours', 'minutes', 'seconds', 'milliseconds'\] - - one of \['utcyear', 'utcquarter', 'utcmonth', 'utcweek', 'utcday', 'utcdayofyear', 'utcdate', 'utchours', 'utcminutes', 'utcseconds', 'utcmilliseconds'\] - - one of \['yearquarter', 'yearquartermonth', 'yearmonth', 'yearmonthdate', 'yearmonthdatehours', 'yearmonthdatehoursminutes', 'yearmonthdatehoursminutesseconds', 'yearweek', 'yearweekday', 'yearweekdayhours', 'yearweekdayhoursminutes', 'yearweekdayhoursminutesseconds', 'yeardayofyear', 'quartermonth', 'monthdate', 'monthdatehours', 'monthdatehoursminutes', 'monthdatehoursminutesseconds', 'weekday', 'weeksdayhours', 'weekdayhoursminutes', 'weekdayhoursminutesseconds', 'dayhours', 'dayhoursminutes', 'dayhoursminutesseconds', 'hoursminutes', 'hoursminutesseconds', 'minutesseconds', 'secondsmilliseconds'\] - - one of \['utcyearquarter', 'utcyearquartermonth', 'utcyearmonth', 'utcyearmonthdate', 'utcyearmonthdatehours', 'utcyearmonthdatehoursminutes', 'utcyearmonthdatehoursminutesseconds', 'utcyearweek', 'utcyearweekday', 'utcyearweekdayhours', 'utcyearweekdayhoursminutes', 'utcyearweekdayhoursminutesseconds', 'utcyeardayofyear', 'utcquartermonth', 'utcmonthdate', 'utcmonthdatehours', 'utcmonthdatehoursminutes', 'utcmonthdatehoursminutesseconds', 'utcweekday', 'utcweeksdayhours', 'utcweekdayhoursminutes', 'utcweekdayhoursminutesseconds', 'utcdayhours', 'utcdayhoursminutes', 'utcdayhoursminutesseconds', 'utchoursminutes', 'utchoursminutesseconds', 'utcminutesseconds', 'utcsecondsmilliseconds'\] - - of type 'object'$""" + - One of \['year', 'quarter', 'month', 'week', 'day', 'dayofyear', 'date', 'hours', 'minutes', 'seconds', 'milliseconds'\] + - One of \['utcyear', 'utcquarter', 'utcmonth', 'utcweek', 'utcday', 'utcdayofyear', 'utcdate', 'utchours', 'utcminutes', 'utcseconds', 'utcmilliseconds'\] + - One of \['yearquarter', 'yearquartermonth', 'yearmonth', 'yearmonthdate', 'yearmonthdatehours', 'yearmonthdatehoursminutes', 'yearmonthdatehoursminutesseconds', 'yearweek', 'yearweekday', 'yearweekdayhours', 'yearweekdayhoursminutes', 'yearweekdayhoursminutesseconds', 'yeardayofyear', 'quartermonth', 'monthdate', 'monthdatehours', 'monthdatehoursminutes', 'monthdatehoursminutesseconds', 'weekday', 'weeksdayhours', 'weekdayhoursminutes', 'weekdayhoursminutesseconds', 'dayhours', 'dayhoursminutes', 'dayhoursminutesseconds', 'hoursminutes', 'hoursminutesseconds', 'minutesseconds', 'secondsmilliseconds'\] + - One of \['utcyearquarter', 'utcyearquartermonth', 'utcyearmonth', 'utcyearmonthdate', 'utcyearmonthdatehours', 'utcyearmonthdatehoursminutes', 'utcyearmonthdatehoursminutesseconds', 'utcyearweek', 'utcyearweekday', 'utcyearweekdayhours', 'utcyearweekdayhoursminutes', 'utcyearweekdayhoursminutesseconds', 'utcyeardayofyear', 'utcquartermonth', 'utcmonthdate', 'utcmonthdatehours', 'utcmonthdatehoursminutes', 'utcmonthdatehoursminutesseconds', 'utcweekday', 'utcweeksdayhours', 'utcweekdayhoursminutes', 'utcweekdayhoursminutesseconds', 'utcdayhours', 'utcdayhoursminutes', 'utcdayhoursminutesseconds', 'utchoursminutes', 'utchoursminutesseconds', 'utcminutesseconds', 'utcsecondsmilliseconds'\] + - Of type 'object'$""" ), ), ( @@ -829,10 +829,10 @@ def chart_error_example__four_errors(): inspect.cleandoc( r"""'invalid_value' is an invalid value for `sort`. Valid values are: - - one of \['ascending', 'descending'\] - - one of \['x', 'y', 'color', 'fill', 'stroke', 'strokeWidth', 'size', 'shape', 'fillOpacity', 'strokeOpacity', 'opacity', 'text'\] - - one of \['-x', '-y', '-color', '-fill', '-stroke', '-strokeWidth', '-size', '-shape', '-fillOpacity', '-strokeOpacity', '-opacity', '-text'\] - - of type 'array', 'object', or 'null'$""" + - One of \['ascending', 'descending'\] + - One of \['x', 'y', 'color', 'fill', 'stroke', 'strokeWidth', 'size', 'shape', 'fillOpacity', 'strokeOpacity', 'opacity', 'text'\] + - One of \['-x', '-y', '-color', '-fill', '-stroke', '-strokeWidth', '-size', '-shape', '-fillOpacity', '-strokeOpacity', '-opacity', '-text'\] + - Of type 'array', 'object', or 'null'$""" ), ), ( diff --git a/tools/schemapi/schemapi.py b/tools/schemapi/schemapi.py index 599f4aaa3..030c4b37b 100644 --- a/tools/schemapi/schemapi.py +++ b/tools/schemapi/schemapi.py @@ -532,6 +532,9 @@ def _get_default_error_message( elif len(bullet_points) == 1: message += f". Valid values are {bullet_points[0]}.\n\n" else: + # We don't use .capitalize below to make the first letter uppercase + # as that makes the rest of the message lowercase + bullet_points = [point[0].upper() + point[1:] for point in bullet_points] message += ". Valid values are:\n\n" message += "\n".join([f"- {point}" for point in bullet_points]) message += "\n\n"