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

Initial extension #74

Closed
wants to merge 3 commits into from
Closed

Initial extension #74

wants to merge 3 commits into from

Conversation

neilsjefferies
Copy link
Member

@neilsjefferies neilsjefferies commented Feb 5, 2024

Add initial placeholder extension definition as per issue #69

julianmorley
julianmorley previously approved these changes Feb 29, 2024
Copy link
Contributor

@zimeon zimeon left a comment

Choose a reason for hiding this comment

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

I think we need to be a little bit more specific about how tooling will understand what functional extension is to followed based on the initial directory. I think the cleanest way to do that is to say that the config.json will give the name of the functional extension.

Also:

  • Suggest we include a revision history at the end because we can't use the obsoletes mechanism with this extension and may have to update it.
  • I don't think we should use the word "placeholder" because it is confusing as to whether the extension document is a placeholde or whether the extension creates a placehoolder space.
  • I think we need a second word for separate from the extension name to make it clear when we are describing the extension versus referring to it. I suggest then name "initial: Extension Initialization"

Here is my stab as what this might look like:

OCFL Community Extension initial: Initial Extension

  • Extension Name: initial
  • Authors: OCFL Editors
  • Minimum OCFL Version: 1.0
  • OCFL Community Extensions Version: 1.0
  • Obsoletes: n/a
  • Obsoleted by: n/a

Overview

This extension allows indication that the semantics of a particular extension should take precedence over all other extensions. It ensures that the special extension name initial is a valid registered extension name and thus that an extension directory initial is also valid in both objects and storage roots.

The contents of the initial extension directory MUST accord with a functional extension as detailed below. The semantics of this functional extension SHOULD be applied before all other extensions.

An initial extension could be used to address otherwise undefined behaviors such as:

  • Should extensions be applied in a specific order?
  • Is an extension deactivated, only applying to earlier versions of the object?
  • Does one extension depend on another?

Parameters

The extension configuration file indicates the function extension to be apply first by specifying that extension's name in the extensionName parameter (not initial).

Example config.json:

{
    "extensionName": "NNNN-functional-extension-name",
    \\ other parameters as defined by the functional extension
}

Revision History


| 2023-03-XX | First published |

@neilsjefferies neilsjefferies dismissed julianmorley’s stale review March 5, 2024 15:26

See Simeon's suggestions...

@neilsjefferies
Copy link
Member Author

neilsjefferies commented Mar 5, 2024

@zimeon So we make initial a real extension whose config identifies the extension that does the initialising. That avoids the placeholder awkwardness...nice.

It does add a level of indirection, though. What do others think?

@rosy1280
Copy link
Contributor

rosy1280 commented Mar 6, 2024

The structure is good but the Parameter section should be fleshed out further or at minimum the grammar should be cleaned up because at present it is hard to understand.

@je4
Copy link
Contributor

je4 commented Mar 13, 2024

From implementation point of view, an initial extension which controls the use of other extension, is not a real extension, but the "extension manager" itself. Having more than one "extension manager" at once would be a very bad idea. Therefor i would support the idea of creating a "real" extension named "initial" with one parameter naming the extension to use as "extension manager".

in my case, this would result in a config like:

{
  "extensionName": "initial",
  "extension": "NNNN-gocfl-extension-manager"
}

This would allow to bootstrap the extension manager with all extensions without violating the ocfl spec.

Another point is, that OCFL says
A root extension directory MAY optionally contain an initial extension that, if it exists, SHOULD be applied before all other extensions in the directory. An initial extension is identified by the extension directory name "initial".
https://github.com/OCFL/extensions?tab=readme-ov-file#optional-initial-extension

It's hard to imagine an initial extension, which is not applied before all other extensions. This means, that the text could be changed to
A root extension directory MAY optionally contain an initial extension that, if it exists, MUST be applied before all other extensions in the directory. An initial extension is identified by the extension directory name "initial".
or that there's a second optional parameter which defines the execution time (before, after, anytime as default).

{
  "extensionName": "initial",
  "extension": "NNNN-gocfl-extension-manager",
  "execute": "before"
}

@zimeon
Copy link
Contributor

zimeon commented Mar 14, 2024

Thanks for your suggestions @je4 ! We discussed in the editors' meeting today https://github.com/OCFL/spec/wiki/2024.03.14-Editors-Meeting and @neilsjefferies will create a revised PR incorporating your suggestions about the first config change and the updated spec language (we won't do the optional execution time).

@@ -1,5 +1,6 @@
# OCFL Community Extensions

* [_initial_ Placeholder](initial.md)
Copy link
Member

Choose a reason for hiding this comment

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

Per @zimeon's comment, the suggestion is to name this: "initial: Initial Extension" or "initial".

* **Obsoletes:** n/a
* **Obsoleted by:** n/a

*Note: This is a placeholder for the special extension name "initial".*
Copy link
Member

Choose a reason for hiding this comment

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

Is this note needed?


## Overview

This extension definition ensures that the special extension name _initial_ is valid registered extension name.
Copy link
Member

Choose a reason for hiding this comment

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

Grammar: ".. initial is valid registered.." -> ".. initial is [a] valid registered.."


## Optional _initial_ Extension

A _root extension directory_ MAY optionally contain an _initial_ extension that, if it exists, SHOULD be applied before all other extensions in the directory.
Copy link
Member

Choose a reason for hiding this comment

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

Based on the discussion in the comments of the PR, the body of this extension should likely be rewritten.

@rosy1280 rosy1280 requested a review from awoods August 29, 2024 16:48
@zimeon zimeon mentioned this pull request Sep 17, 2024
@neilsjefferies
Copy link
Member Author

Superseded by #80

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.

6 participants