Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Entry validation methods #28

Merged
merged 8 commits into from
Jun 10, 2024

Conversation

shilorigins
Copy link
Collaborator

@shilorigins shilorigins commented May 14, 2024

Description

  • Add .validate() method for all Entry subclasses
  • Introduce Nestable interface for Collection and Snapshot

Motivation and Context

Validation ensures that Entrys are well formed before certain uses, such as inserting into a backend: close #11

Adding validation methods to each Entry subclass leverages polymorphism to keep validation code close to the data being validated. This also keeps the client or backend agnostic to the type of Entry being validated.

Entry.validate() uses an apischema serialization roundtrip to perform type checks. This implementation captures ValidationErrors and returns False if type checks fail, but I'd be open to leaving the exception if that's preferred. If a subclass needs specific validation tests, it can override this method; the subclass is responsible for calling Entry.validate() or otherwise performing type checks.

Nestable reduces code duplication between Collection and Snapshot, and encourages more generalized code when working with these types.

How Has This Been Tested?

Ran pytest from within the repo. Used linac_backend and new linac_snapshot fixtures to validate the original trees, a tree with an empty Collection, a tree with a cycle, and class instances that fail type checks.

Test coverage
Name                    Stmts   Miss  Cover
-------------------------------------------
__init__.py                 2      0   100%
__main__.py                 2      2     0%
_version.py                11     11     0%
backends/__init__.py        0      0   100%
backends/core.py           14      5    64%
backends/filestore.py     147     22    85%
backends/test.py           36      0   100%
bin/__init__.py             2      2     0%
bin/help.py                11     11     0%
bin/main.py                51     51     0%
bin/ui.py                  13     13     0%
client.py                  22     22     0%
errors.py                   6      0   100%
model.py                  153      6    96%
serialization.py           61     19    69%
tests/__init__.py           0      0   100%
tests/conftest.py         107      0   100%
tests/test_backend.py      71      0   100%
tests/test_model.py        79      0   100%
tests/test_window.py        5      0   100%
type_hints.py               2      0   100%
utils.py                   11      4    64%
version.py                 27     17    37%
widgets/__init__.py         0      0   100%
widgets/core.py             5      0   100%
widgets/window.py           5      0   100%
-------------------------------------------
TOTAL                     843    185    78%
Test durations
0.05s call     superscore/tests/test_window.py::test_main_window
0.04s setup    superscore/tests/test_window.py::test_main_window
0.04s call     superscore/tests/test_backend.py::test_save_entry[0]
0.02s call     superscore/tests/test_model.py::TestEntryValidation::test_fixture_validation
0.01s call     superscore/tests/test_model.py::test_serialize_snapshot_roundtrip
0.01s call     superscore/tests/test_model.py::test_serialize_collection_roundtrip
0.01s call     superscore/tests/test_backend.py::test_search_entry[0]

(47 durations < 0.005s hidden.  Use -vv to show these durations.)

Where Has This Been Documented?

This PR

Pre-merge checklist

  • Code works interactively
  • Code follows the style guide
  • Code contains descriptive docstrings, including context and API
  • New/changed functions and methods are covered in the test suite where possible
  • Test suite passes locally
  • Test suite passes on GitHub Actions
  • Ran docs/pre-release-notes.sh and created a pre-release documentation page
  • Pre-release docs include context, functional descriptions, and contributors as appropriate

@shilorigins shilorigins self-assigned this May 14, 2024
@shilorigins shilorigins force-pushed the devagr/entry-validation branch 2 times, most recently from 32897b6 to 9a8122f Compare May 17, 2024 15:38
@shilorigins shilorigins force-pushed the devagr/entry-validation branch 3 times, most recently from 61871f2 to 6381b7d Compare May 31, 2024 16:04
@shilorigins shilorigins force-pushed the devagr/entry-validation branch from 6381b7d to e88904d Compare May 31, 2024 16:13
@shilorigins shilorigins marked this pull request as ready for review May 31, 2024 16:15
@shilorigins shilorigins requested review from tangkong and ZLLentz May 31, 2024 16:15
docs/source/upcoming_release_notes/28-entry_validation.rst Outdated Show resolved Hide resolved
superscore/tests/test_model.py Show resolved Hide resolved
superscore/tests/test_model.py Outdated Show resolved Hide resolved
superscore/model.py Outdated Show resolved Hide resolved
superscore/model.py Outdated Show resolved Hide resolved
superscore/model.py Outdated Show resolved Hide resolved
@shilorigins shilorigins force-pushed the devagr/entry-validation branch 2 times, most recently from 17573c5 to eb4cfa4 Compare June 4, 2024 22:05
@shilorigins shilorigins requested a review from tangkong June 7, 2024 15:24
superscore/model.py Outdated Show resolved Hide resolved
superscore/model.py Outdated Show resolved Hide resolved
superscore/model.py Outdated Show resolved Hide resolved
Entry.validate includes an apischema serialization roundtrip to enforce
type checks

Nestable validation includes cycle detection and ensuring Nestables aren't
empty.
After confirming that a Collecion with a cycle doesn't validate,
confirm whether the fixed Collection does validate.

The initial implementation of Nestable.has_cycle used a mutable
default parameter, meaning that call results could be influenced
by previous calls. This addition tests that similar behaviour
isn't re-introduced.
The previous implementation used one "parents" set for an
entire tree, with each node mutating the set before its
child calls and again before returning. Now each node
creates a new set to pass to all of its children, limiting
scope of each set.
Previously, Nestables would fail validation if they didn't have
any children. Such entries are now considered valid, in case users
want to record meta_pv values without any recording any restorable
values.
@shilorigins shilorigins force-pushed the devagr/entry-validation branch from eb4cfa4 to 97331fa Compare June 7, 2024 22:52
@ZLLentz
Copy link
Member

ZLLentz commented Jun 10, 2024

I'm here a little late but everything here looks solid to me
I'll skip hitting approve myself because I think Robert's approval is needed following his previous review

Copy link
Contributor

@tangkong tangkong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@shilorigins shilorigins merged commit 6073b4d into pcdshub:master Jun 10, 2024
11 checks passed
@shilorigins shilorigins deleted the devagr/entry-validation branch June 10, 2024 22:31
@tangkong tangkong linked an issue Jan 21, 2025 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build Entry validation logic Integrate basic CRUD methods with Backend API
3 participants