-
Notifications
You must be signed in to change notification settings - Fork 37
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
Make synced collection a hidden package #807
Changes from all commits
7a77815
e532fc1
fcd3c95
da50adf
e096e2a
ed307e0
0279ce0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,10 +15,10 @@ | |
|
||
import os | ||
|
||
from signac._synced_collections.backends.collection_json import BufferedJSONAttrDict | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we should replace all references to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We also use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I say let's leave this as is for now. If and when we get around to making |
||
from signac.common import configobj | ||
from signac.common.config import _get_project_config_fn | ||
from signac.contrib.project import Project | ||
from signac.synced_collections.backends.collection_json import BufferedJSONAttrDict | ||
|
||
from .v0_to_v1 import _load_config_v1 | ||
|
||
|
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.
This class reference won't work because we're removing the synced collection docs. I wonder if we should add docs for the publicly imported name
signac.JSONDict
and point to that instead.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.
Won't the docs for JSONDict (which is buffered) and JSONAttrDict be different?
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.
😕 This is tricky. We want to just point to public docs for a standalone synced collections package, but if we make synced collections hidden, we can't point to these docs.
Maybe this entire approach is flawed and we should leave
signac.synced_collections
as a public subpackage until we makesynced_collections
standalone. Maybe this is a signac 3.0 task and we should leave a public subpackage in 2.0.cc: @glotzerlab/signac-committers
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.
See my comment on the corresponding issue. Even if
synced_collections
stays public, I think pointing to a specific class here is a flawed approach. From a user's perspective, do they need any information other than the fact that this attribute is a dict-like object? If we really want to explain the syncing, I would say that we could point to the parentSyncedDict
class, but IMO that's exposing implementation details because synchronization is transparent.There is only one minor exception that comes to my mind immediately, which is that the document currently supports buffering on a per-document basis:
but TBH I'm not sure that's something that we really need to promise at the signac level. The
signac.buffered
API should be the only entrypoint for buffering for signac users IMO:since that abstracts over the details of specific backends and all instances. That abstraction guarantees that in a world where signac starts supporting different backends, users still have a single switch in the API to toggle when reads and writes are maximally buffered, and it's up to the implementation of signac to actually perform that buffering to the best of its ability. So I think that ultimately the only API promise (and therefore all that we should document) is that
[job|project].[sp|doc]
areMutableMapping
s, and perhaps that they support attribute-based access. We shouldn't link to a type because we don't actually promise the type of the object (again, see #790 (comment) for why).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 agree with this approach. Thanks @vyasr. @kidrahahjo does this make sense on what to change? If not, I can take a look at changing it myself and push to this PR.
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 far as I understood this, we do not need to expose any of the implementation details so we can either not specify the type or use MutableMappings (which is another way of saying its a dict like object) instead?