Skip to content

chore: make _path optional and more representative #4303

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

Merged
merged 3 commits into from
Jul 7, 2025

Conversation

benfdking
Copy link
Contributor

@benfdking benfdking commented May 2, 2025

At the moment, the path type is slightly confusing. Models can exist without a Path. Rather than mark the _path property as None we call Path() which is equivalent to calling Path(".") which is not particularly indicative it just returns your path to cwd. Optional is a much better representation.

@benfdking benfdking marked this pull request as draft May 2, 2025 16:45
@benfdking benfdking force-pushed the rough_making_model_node_optional branch from 4b06000 to 7caa5fa Compare May 15, 2025 10:45
@benfdking benfdking marked this pull request as ready for review May 15, 2025 10:45
@benfdking benfdking changed the title NOT READY: chore: make _path optional chore: make _path optional and thus more representative May 15, 2025
@benfdking benfdking force-pushed the rough_making_model_node_optional branch from 7caa5fa to ac68e4c Compare June 4, 2025 10:45
@benfdking benfdking force-pushed the rough_making_model_node_optional branch from aab1108 to b7be02d Compare June 11, 2025 19:33
@benfdking benfdking changed the title chore: make _path optional and thus more representative chore: make _path optional and more representative Jun 12, 2025
@benfdking benfdking force-pushed the rough_making_model_node_optional branch from b7be02d to f25a7ee Compare June 17, 2025 11:23
@benfdking benfdking force-pushed the rough_making_model_node_optional branch 3 times, most recently from 7055cb5 to 98b2d25 Compare July 4, 2025 17:26
@benfdking benfdking force-pushed the rough_making_model_node_optional branch from 98b2d25 to 7e427f7 Compare July 7, 2025 11:06
Copy link
Contributor

@georgesittas georgesittas left a comment

Choose a reason for hiding this comment

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

Seems good, just some minor comments from me.

Comment on lines +301 to +298
if audit._path is None:
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we warn? Seems like we'd otherwise drop the audit silently.

benfdking added 3 commits July 7, 2025 16:07
- more correctly represent path being optional
@benfdking benfdking force-pushed the rough_making_model_node_optional branch from 7e427f7 to a340311 Compare July 7, 2025 15:11
@benfdking benfdking merged commit a412110 into main Jul 7, 2025
27 checks passed
@benfdking benfdking deleted the rough_making_model_node_optional branch July 7, 2025 17:43
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.

3 participants