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 *INCOMPLETE* working revision of specification document #170

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ljnelson
Copy link
Contributor

No description provided.

@ljnelson
Copy link
Contributor Author

ljnelson commented Apr 13, 2023

This pull request begins to flesh out what a specification should start to look like. There is a ridiculously long way to go.

(It incorporates work previously done in #151 (and mostly agreed upon on phone calls although the agreement was never captured in minutes or our discussion areas or the mailing list) which has been closed in favor of this PR, which will be ongoing.)

All (hopefully rigorous and precise) words are drawn from (mostly not rigorous or precise) discussions we've had in the past, some of which, but not all, are reachable from the discussions area in this project, some of which, but not all, are inferable from our sparsely-kept meeting minutes, and sadly none of which are present, by majority opinion, on our mailing list for some reason. I've tried diligently to capture the spirit of what I thought people were trying to say. In some cases it was very hard. My point here is: I haven't "snuck anything in". This is all stuff we have said directly or implied. Really.

💥 📣 If you look at this PR only by reading the .adoc files on Github's deliberately awful crippled Asciidoc UI, they will look bad, and I will know you haven't read these instructions.

The specification as it appears in this PR is INCOMPLETE. I still have work to do. If you don't see your Special Pet Subject™ in it, that doesn't necessarily mean I've ignored it. It either means (a) we never discussed it or, more likely, never agreed on it, in which case it doesn't go in the spec yet, or (b) I haven't had time to put it in yet.

To evaluate this PR, please check it out and build the project (at least mvn generate-resources). Then look for spec/target/generated-docs/jakarta-config-specification.html.

I've also attached a tarball containing the generated HTML files:

pr-170.tar.gz

@ljnelson ljnelson marked this pull request as draft April 13, 2023 04:12
* <p><strong>\u26A0 Caution:</strong> you are reading an incomplete
* draft specification that is subject to change.</p>
*/
public class InvalidConfigurationClassException extends ConfigException {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just InvalidConfigurationException?

@@ -48,11 +51,11 @@
public interface Loader {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if this should be ConfigLoader or ConfigurationLoader?

* not found.
*
* <p><strong>\u26A0 Caution:</strong> you are reading an incomplete
* draft specification that is subject to change.</p>
*/
public class NoSuchObjectException extends ConfigException {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe use NoSuchElementException instead of creating a new exception?

Comment on lines 68 to 79
. A `public` instance method of a configuration class may declare parameters.

.. If a `public`, non-`default` instance method of a configuration class declares parameters, and the configuration
class is supplied as part of a load request, a Jakarta Config implementation may choose to either:

... Implement the method somehow, provided that it fulfils all other structural requirements, or

... Implement the method such that it throws either:

.... a `jakarta.config.NoSuchObjectException`, or

.... a `java.lang.UnsupportedOperationException`
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 we can simplify this, and just don't allow instance methods with parameters.

<5> This configuration class, considered apart from its members, and apart from its declaring class, represents a
configuration to which the configuration paths it represents are relative.

<6> This method, `partNumbers()`, considered on its own, represents a single-element configuration path, comprising a
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if the representation of the name should be in camelCase. For instance, JPA Jakarta properties use kebab-case.

@ljnelson ljnelson marked this pull request as ready for review August 2, 2023 22:58
Comment on lines 214 to 216
System.getLogger(Loader.class.getName())
.log(System.Logger.Level.DEBUG, absentValueException::getMessage, absentValueException);
.log(System.Logger.Level.DEBUG, e::getMessage, e);
return loader;
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this throw an InvalidConfigurationException?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or maybe NoSuchElementException?

Comment on lines +199 to +201
<6> This method, `partNumbers()`, considered on its own, represents a single-element configuration path, comprising a
single configuration key whose canonical representation is `partNumbers`, relative to the configuration represented by
its declaring class, that (hopefully) resolves to a `List<Integer>`-typed representation of a configuration value.
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to clarify how each element in the list is represented as well. My proposal is that each element name is determined by enclosing their index in square brackets, like [0].


CAUTION: You are reading an incomplete DRAFT specification. The following is subject to change.

== Overview
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems this section is very similar to the Goals section. Maybe they can be merged?

Comment on lines +54 to +64
<1> The `MyConfigurationObject.class` argument is a _xref:terminology.adoc#configuration-interface[configuration
interface]_ and here represents a _xref:terminology.adoc#load-request[load request]_.

. *The implementor's `jakarta.config.Loader` implementation builds the object and returns it.*

.. If the implementor's `jakarta.config.Loader` implementation determines that the supplied load request either:
... does not identify a configuration object, or
... would result in an invalid configuration object

+
…it throws a `jakarta.config.NoSuchObjectException`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the goal to specify if such objects are subject to pre-validation? For instance, the Config system, discovers or registers candidates or is it something that happens on the fly (when calling the loader to load)?

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.

2 participants