Add config validation#83
Conversation
Add a validate() function to nps_active_space/utils/config.py that checks the loaded configuration for structural correctness: - Verifies all expected sections exist (database:overflights, data, project) - Confirms each section contains its expected keys - Flags unknown sections or keys as potential typos - Checks that required values (project.dir) are non-empty - Validates that non-empty path values point to existing files or directories - Returns a list of human-readable error strings (empty = valid) - Optionally prints each problem as it's found (verbose=True) The schema is defined as module-level constants (EXPECTED_SCHEMA, _PATH_KEYS, _REQUIRED_VALUES) derived from the template.config and actual usage across all scripts in the codebase. Includes 15 unit tests covering happy path, uninitialized config, missing sections, missing keys, unknown sections/keys, required values, path existence, and verbose output behavior. Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
elliott-ruebush
left a comment
There was a problem hiding this comment.
Left a couple small questions
One bigger picture question as well. Where do you think would be best to do validations moving forward? IMO in an ideal world we would validate the relevant sections for each script that reads the config, but that could be a bit complex, so I'm tempted to say we just validate on each read and allow empty elements for config items that might not be required for every situation
| loaded_sections = set(_config.sections()) | ||
| expected_sections = set(EXPECTED_SCHEMA.keys()) | ||
|
|
||
| # --- unknown sections (typo detection) --- |
There was a problem hiding this comment.
Could we split these different sections into their own smaller functions so that the method reads clearer overall?
e.g. _check_unknown_sections
Clear method naming would mean we don't need these comments on each section either.
There was a problem hiding this comment.
Done. Pushed a refactor that splits validate() into _check_loaded(), _check_unknown_sections(), _check_missing_sections(), and _check_section_keys(). The method names make the comments redundant so I dropped those too.
| def test_empty_project_dir(self, tmp_path): | ||
| _write_config(tmp_path, VALID_CONFIG.format(project_dir="")) | ||
| errors = cfg.validate(verbose=False) | ||
| assert any( |
There was a problem hiding this comment.
Do you think there's value in doing an assertion on the full errors result list to confirm we didn't have additional unexpected errors? Or do you think it's cleaner to verify just the expected error's existence for these unit tests?
There was a problem hiding this comment.
I went with full-list assertions on the errors result. If someone adds a new required key later and the validation starts catching it, the test will surface it immediately rather than silently passing. Feels worth the small extra maintenance cost of updating the expected list when the schema changes.
|
Validating on read feels right to me too. Catches problems early, keeps the logic in one place, and we don't have to coordinate between scripts. If some config keys end up being optional depending on the use case, we can add an optional_keys param to validate() later without reworking things. I'll push the refactor (smaller named functions) and updated test assertions shortly. |
Signed-off-by: arpitjain099 <arpitjain099@gmail.com>
elliott-ruebush
left a comment
There was a problem hiding this comment.
Approving, although as is there's not anything actually utilizing the validation in the pieces of code where configs are loaded. This could come in a follow-up (I had looked some at refactoring the config loading in some separate changes)
Closes #64.
This adds a
validate()function tonps_active_space/utils/config.pythat catches config problems before they surface as confusing runtime errors.What it checks
database:overflights,data,project) are presenttemplate.configand actual usage across every script in the codebase)[project] diris non-empty (it's used by nearly every script)The function returns a list of plain-English error strings. An empty list means the config looks good. There's also a
verboseflag (on by default) that prints each problem as it's found, so the user sees exactly what to fix and where.The schema is defined as module-level constants (
EXPECTED_SCHEMA,_PATH_KEYS,_REQUIRED_VALUES) so it's easy to extend as new config keys get added. Key names are lowercase becauseConfigParserlowercases them internally.Tests
15 tests in
tests/utils/test_config_validation.pycovering:project.dir)All 15 pass locally.
Notes
I kept this as a standalone function rather than auto-running on every
initialize()call. That way existing scripts aren't affected, and users can callcfg.validate()when they want to (or it can be wired into a CI step or CLI check later, per the ideas in #64). The DB connection check is left out for now, same reasoning as in the issue.