abravalheri / validate-pyproject

Validation library for simple check on `pyproject.toml`
https://validate-pyproject.readthedocs.io/
Mozilla Public License 2.0
129 stars 12 forks source link

Modifying dictionary? #197

Closed henryiii closed 2 weeks ago

henryiii commented 2 months ago

It looks like this modifies the input dictionary, filling in the defaults. That's why some checks are strongly failing for repo-review; repo-review assumes the input dictionary is not modified. There are three possible fixes:

  1. Stop modifying the pyproject structure
  2. Fix the repo-review check so that a deep copy is made before processing
  3. Have repo-review give every check a deep copy

I'd much rather at least Option 2 over Option 3, but maybe Option 1 should be investigated first?

Before: ```python {'disallow_untyped_defs': False, 'enable_error_code': ['ignore-without-code', 'redundant-expr', 'truthy-bool'], 'files': ['src', 'web', 'tests'], 'mypy_path': ['src'], 'overrides': [{'disallow_untyped_defs': True, 'module': 'repo_review.*'}], 'python_version': '3.10', 'strict': True, 'warn_unreachable': True, 'warn_unused_configs': True} ```
After: ```python {'allow_redefinition': False, 'allow_untyped_globals': False, 'cache_dir': '.mypy_cache', 'cache_fine_grained': False, 'check_untyped_defs': False, 'color_output': True, 'disallow_any_decorated': False, 'disallow_any_explicit': False, 'disallow_any_expr': False, 'disallow_any_generics': False, 'disallow_any_unimported': False, 'disallow_incomplete_defs': False, 'disallow_subclassing_any': False, 'disallow_untyped_calls': False, 'disallow_untyped_decorators': False, 'disallow_untyped_defs': False, 'enable_error_code': ['ignore-without-code', 'redundant-expr', 'truthy-bool'], 'error_summary': True, 'explicit_package_bases': False, 'extra_checks': False, 'files': ['src', 'web', 'tests'], 'follow_imports': 'normal', 'follow_imports_for_stubs': False, 'force_union_syntax': False, 'force_uppercase_builtins': False, 'hide_error_codes': False, 'ignore_errors': False, 'ignore_missing_imports': False, 'implicit_optional': False, 'implicit_reexport': True, 'incremental': True, 'local_partial_types': False, 'mypy_path': ['src'], 'namespace_packages': True, 'no_implicit_optional': True, 'no_implicit_reexport': False, 'no_silence_site_packages': False, 'no_site_packages': False, 'overrides': [{'disallow_untyped_defs': True, 'module': 'repo_review.*'}], 'pdb': False, 'pretty': False, 'python_version': '3.10', 'raise_exceptions': False, 'scripts_are_modules': False, 'show_absolute_path': False, 'show_column_numbers': False, 'show_error_code_links': False, 'show_error_codes': True, 'show_error_context': False, 'show_traceback': False, 'skip_cache_mtime_checks': False, 'skip_version_check': False, 'sqlite_cache': False, 'strict': True, 'strict_concatenate': False, 'strict_equality': False, 'strict_optional': True, 'verbosity': 0, 'warn_incomplete_stub': False, 'warn_no_return': True, 'warn_redundant_casts': False, 'warn_return_any': False, 'warn_unreachable': True, 'warn_unused_configs': True, 'warn_unused_ignores': False} ```
henryiii commented 2 months ago

I was able to intelligently go with Option 3, where it only makes another copy if the input changes (at the expense of making at least one copy and doing a comparison every check). So now this will warn unless 1 or 2 is done, but will work correctly and not break other checks.

abravalheri commented 1 month ago

Thank you very much for reporting this @henryiii.

Probably this is related on how the fastjsonschema dependency works, and in that case it would be difficult to change.

A viable approach is to add a deepcopy in validate_pyproject directly, just before running the checks (there might be some small loss in terms of performance which could be counterbalanced if we make the deepcopy opt-in/opt-out).

Do you have any thoughts on that?

henryiii commented 2 weeks ago

Maybe it would be worth asking there first, then?

Documenting this and letting a user decide to pay the cost for the deepcopy would probably be fine - much of the time, it's probably fine to let it mutate, since often it's not being used after being validated. It's only needed if you want to use it afterwards.