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

File references #462

Merged
merged 18 commits into from
May 11, 2020
Merged

File references #462

merged 18 commits into from
May 11, 2020

Conversation

achilleas-k
Copy link
Member

@achilleas-k achilleas-k commented May 4, 2020

Every object that subclasses Entity now holds a reference to the nix.File object that they belong to. This reference is the same object for all. The reference is useful for a few reasons but more recently it becomes necessary for being able to read the auto_update_timestamps option, which is a session option only (it's not stored in the file) and is only available through the original File object.

Fixes #460

Changes in this PR:

  • The timestamp update option, introduced in PR Add auto-time-update flag to NIXpy file #385 has been renamed to auto_update_timestamps. When enabled, it updates the updated_at timestamp for an object when one of its properties are changed.
  • The option is now ENABLED by default. The thought behind having an option to disable the timestamp was that fast creation and/or editing of a large number of objects would incur a significant overhead when each operation required editing the object timestamp. The property was always meant to be updated when an object changes, but the library (or libraries, NIX C++ included) were not consistent in their behaviour across objects and properties. Now that we've increased the scope of this behaviour, I think the default should still be to keep updating timestamps but allow users to disable the behaviour for performance reasons if they expect it will help. This should be documented in the file creation/opening docs.
  • Added tests for timestamp updating enabled and disabled. These tests can be a bit slower than others since they require waiting for time to pass to check the time change.

NOTE: The NIX compatibility tests will fail until we fix ASCII reading (see G-Node/nix#817).

The creator method is meant to be used by parent classes.  I had
originally made it "private" to imply it's for internal library use
only.

In the future, we might fold the creation functionality into the
__init__ function along with the instantiation of objects from a file.
All Entities are initialised with and keep a reference to the File
object.
Containers keep the same reference to initialise objects that it
instantiates from the file.
Non-Entity objects that also require this reference:
- Feature
- Dimension objects
Testing in container for convenience (has all objects) and because
instantiating objects through containers is where an object might lose
references to parent objects.
Type and Definition changes on Block.
- Rename 'time_auto_update' to 'auto_update_timestamp'
- Property and constructor argument are the same
- Enabled by default: Updating timestamps should be the default.  The
  overhead is probably minimal (perhaps even negligible) and should only
  be disabled for performance when required.
Instead of separating based on property, separate based on option state.
@lgtm-com
Copy link

lgtm-com bot commented May 4, 2020

This pull request introduces 2 alerts when merging 2fc194e into 9b63d54 - view on LGTM.com

new alerts:

  • 2 for Wrong number of arguments in a class instantiation

- Removed hacky parent traversal for getting root objects like the top
  level metadata container.
- Removed H5Group utility functions that are now obsolete.
- Containers that need to do a search from the root object to delete
  objects now use the file reference.
Copy link
Member

@jgrewe jgrewe left a comment

Choose a reason for hiding this comment

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

lgtm

@jgrewe jgrewe merged commit 7ce8de0 into G-Node:master May 11, 2020
@achilleas-k achilleas-k deleted the file-references branch May 11, 2020 13:26
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.

Parent references not set for obj.metadata
2 participants