-
Notifications
You must be signed in to change notification settings - Fork 64
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
Added SupportsNamespaces
interface
#1280
Conversation
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## master #1280 +/- ##
==========================================
- Coverage 71.26% 0.00% -71.27%
==========================================
Files 277 279 +2
Lines 13586 13610 +24
==========================================
- Hits 9682 0 -9682
- Misses 3904 13610 +9706
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
ee82a81
to
3209399
Compare
|
||
@abstractmethod | ||
def __delitem__(self, path) -> None: | ||
... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add get_root_object
to the interface? + to Run
as something like
def get_root_object(self):
return self
This is something we use in many integrations and having it as a part of interface would help to avoid unnecessary type checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you wish. Done
src/neptune/integrations/utils.py
Outdated
@@ -13,7 +13,7 @@ | |||
# See the License for the specific language governing permissions and | |||
# limitations under the License. | |||
# | |||
__all__ = ["expect_not_an_experiment", "join_paths", "verify_type", "RunType"] | |||
__all__ = ["expect_not_an_experiment", "join_paths", "verify_type", "RunType", "SupportsNamespaces"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want it to be importable from neptune.integration.utils
? Why not just use neptune.metadata_containers.abstract
? Even if we want to keep the neptune.metadata_containers.abstract
private, maybe it'd make sense to have it in some kind of "types" or "interfaces" module, it's not something I'll look for in integration.utils
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I've exposed it via neptune.typing
. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it, but left few small comments on possible improvements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Before submitting checklist