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

Get rid of DUMMY_NONEXISTING_PATH #479

Merged
merged 5 commits into from
Feb 17, 2025
Merged

Get rid of DUMMY_NONEXISTING_PATH #479

merged 5 commits into from
Feb 17, 2025

Conversation

J08nY
Copy link
Member

@J08nY J08nY commented Feb 15, 2025

Fixes #290.

@J08nY J08nY force-pushed the fix/no-dummy-path branch from 15e63d4 to c06dc52 Compare February 15, 2025 20:41
@J08nY J08nY added the refactoring Need to cleanup and refactor label Feb 15, 2025
@J08nY J08nY requested a review from adamjanovsky February 15, 2025 21:15
@J08nY J08nY self-assigned this Feb 15, 2025
@J08nY J08nY marked this pull request as ready for review February 15, 2025 21:15
@J08nY
Copy link
Member Author

J08nY commented Feb 15, 2025

@adamjanovsky What do you think about this? It gets rid of the DUMMY_NONEXISTING_PATH. The cost is a few ignores in typing. However, I would argue that those are quite reasonable, as before the distinction of whether something is a dummy path or not was not done on type level anyway. So on a static level we do not gain anything and on a runtime level it is a bit more clear what is going on and we have no chance to get errors where we try to write somewhere we shouldn't.

Copy link
Collaborator

@adamjanovsky adamjanovsky left a comment

Choose a reason for hiding this comment

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

Sure, that's a step in a good direction. I provided some individual comments. I also think that a release would be nice once you're finished with these tiny-refactoring PRs.

@@ -65,7 +65,7 @@ def __init__(
self.name = name if name else type(self).__name__
self.description = description if description else datetime.now().strftime("%d/%m/%Y %H:%M:%S")
self.state = state if state else self.DatasetInternalState()
self.root_dir = Path(root_dir)
self.root_dir = Path(root_dir) if root_dir is not None else None # type: ignore
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is the # type: ignore required? Shouldn't self.root_dir expect Path | None assignments?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, it should, because that is what the setter of the property takes. However, since the getter is annotated with just Path mypy is not happy here no matter what I do...

@@ -30,8 +29,8 @@ class AuxiliaryDatasetHandler(ABC):
aux_datasets_dir: Path
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just set this to Path | None to avoid the type warnings?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, we need to lie here as well I think. The same way we are lying in the Dataset.root_dir property claiming that it is never None and only protecting it via the only_backed decorator. Otherwise, typewise it just all goes wrong and every access to any attribute derived from root_dir needs to be checked and guarded.

@J08nY J08nY merged commit 27abe1d into main Feb 17, 2025
4 checks passed
@J08nY J08nY deleted the fix/no-dummy-path branch February 17, 2025 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Need to cleanup and refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None instead of DUMMY_NONEXISTING_PATH when path not set on Dataset instances
2 participants