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

Definition tuning #158

Merged
merged 8 commits into from
Oct 8, 2018
Merged

Definition tuning #158

merged 8 commits into from
Oct 8, 2018

Conversation

ahankinson
Copy link
Contributor

  • Reformatted definitions to follow the style of the rest of the spec.
  • Reworded some definitions to make their intent clearer
  • Added missing definitions

 - Reformatted definitions to follow the style of the rest of the spec.
 - Reworded some definitions to make their intent clearer
 - Added missing definitions
<dd>The state of an <a>OCFL object</a>'s content which is constructed using the incremental
changes recorded in the sequence of corresponding and prior version directories.</dd>
<dd>
The file path as it appears on disk including the version directories and the filename, and relative to
Copy link
Member

Choose a reason for hiding this comment

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

The wording here is a bit odd:

including the version directories and the filename, and relative to an <a>OCFL Object Root</a>.

Copy link
Contributor

Choose a reason for hiding this comment

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

The addition of version directories rather than directories also ignores the potential for directories within the OCFL object itself.

<dfn>Digest</dfn>:
</dt>
<dd>
An algorithmic characterisation of the contents of a file conforming to a standard digest algorithm.
Copy link
Member

Choose a reason for hiding this comment

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

U.S. spelling characterization

<dfn>Manifest</dfn>
</dt>
<dd>
A complete listing of all files, with their digests, within an OCFL Object and stored in the
Copy link
Member

Choose a reason for hiding this comment

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

It would read better for me if the and stored in the <a>Inventory</a> came earlier. Such as:

A section of the <a>Inventory</a> containing a complete listing...

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Contributor

@rosy1280 rosy1280 left a comment

Choose a reason for hiding this comment

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

@zimeon at some point during the F2F meeting in 2018.09 you made a request to keep definitions to one sentence. Some of the changes I'm requesting are because of that. Can you elaborate on that need, or can we forego it...

draft/spec/index.html Outdated Show resolved Hide resolved
<dfn>Manifest</dfn>
</dt>
<dd>
A complete listing of all files, with their digests, within an OCFL Object and stored in the
Copy link
Contributor

Choose a reason for hiding this comment

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

+1

<dd>The state of an <a>OCFL object</a>'s content which is constructed using the incremental
changes recorded in the sequence of corresponding and prior version directories.</dd>
<dd>
The file path as it appears on disk including the version directories and the filename, and relative to
Copy link
Contributor

Choose a reason for hiding this comment

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

The addition of version directories rather than directories also ignores the potential for directories within the OCFL object itself.

</dt>
<dd>
An algorithmic characterisation of the contents of a file conforming to a standard digest algorithm.
Digests are used for recording fixity and for content-addressable storage within an OCFL Object.
Copy link
Contributor

Choose a reason for hiding this comment

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

Again two sentences...

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the second sentence is actually slightly misleading now... we don't really use content-addressable storage in the usual sense (the files most likely have their original names), instead we have a kind of content reference in the inventory. My sense is that the two sentences are short so I'd be fine to keep them, but would change "content-addressable storage" to "content references"

<dd>
The base directory of an <a>OCFL Object</a> that may exist either as a plain directory, or in the
case of a zipped OCFL Object, as the top directory in a zip file. The Object Root is identified by
the presence of a NamAsTe file, "0=ocfl_object_1.0".
Copy link
Contributor

Choose a reason for hiding this comment

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

Again two sentences....

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO the second sentence isn't required here, it is repeating part of the spec

@zimeon
Copy link
Contributor

zimeon commented Sep 19, 2018

@rosy1280 I think the key is to keep the definitions short, usually one sentence. However, if one sentence really doesn't work then I would not force awkward language or forgo clarity to follow that rule

@ahankinson
Copy link
Contributor Author

Thanks for the comments -- I've re-worked the definitions according to feedback, with one exception: I kept the reference to the NamAsTe file in OCFL Object Root since it a) mirrors that of the one in OCFL Storage Root, and b) an OCFL Object Root can be identified as such by its presence, so it forms part of the identity (which seems important to note in the definition)

<dfn>OCFL Object Root</dfn>:
<dt>
<dd>
The base directory of an <a>OCFL Object</a>, identified by a NamAsTe file "0=ocfl_object_1.0".
Copy link
Contributor

@zimeon zimeon Oct 5, 2018

Choose a reason for hiding this comment

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

[[NAMSATE]] not NamAsTe? Good to link and the spec doesn't use the CamelCase version (though does explain as "NAMe AS TExt")

<dfn>OCFL Storage Root</dfn>:
</dt>
<dd>
A base directory used to store OCFL Objects, identified by a NamAsTe file "0=ocfl_1.0".
Copy link
Contributor

Choose a reason for hiding this comment

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

similarly: [[NAMASTE]]

<dd>The state of an <a>OCFL object</a>'s content which is constructed using the incremental changes recorded in
the sequence of corresponding and prior version directories.</dd>
<dd>
The file path of a file on disk, relative to the <a>OCFL Object
Copy link
Contributor

Choose a reason for hiding this comment

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

... of a file on disk or in an object store, ...

zimeon
zimeon previously approved these changes Oct 5, 2018
<dfn>Inventory</dfn>:
</dt>
<dd>
A file, expressed in JSON-LD, that tracks the history and current
Copy link
Contributor

Choose a reason for hiding this comment

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

No JSON-LD

Copy link
Contributor

Choose a reason for hiding this comment

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

Well spotted!

@zimeon zimeon merged commit 2f8c45e into master Oct 8, 2018
@zimeon zimeon deleted the tuning-definitions branch October 8, 2018 17:37
@ahankinson
Copy link
Contributor Author

🎉

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.

5 participants