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

Expand criterium for consumed APIs; mention requires/okapiInterfaces/... #75

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

julianladisch
Copy link
Contributor

This is the current acceptance criterium:

- * Module descriptor MUST include interface requirements for all consumed APIs (3, 5)
-   * _This is not applicable to libraries_

This should be more actionable.

Evaluators need to browse through the source code to find out the consumed APIs and then check that the API is listed. Therefore the parts of the sentence should be swapped to start with "For each consumed API".

Evaluators of backend modules need to look up the interface requirement in the "requires" or "optional" section of the module descriptor. The section names should be mentioned in the criterium to make it more actionable. The name references the relevant part of the module descriptor documentation making the criterium easier to understand (the preceding criterium links to the module descriptor documentation). The criterium about the provided interfaces already mentions the "provides" section, similarly the the "requires" and "optional" sections should be mentioned.

Currently the criterium about the interface requirements in the module descriptor also applies to frontend modules. This can be simplified because the build process generates the module descriptor from the package.json file, and there's already a criterium that it "MUST produce a valid module descriptor".

Therefore we can change the criterium for the frontend to require the interfaces of the consumed APIs to be in the "okapiInterfaces" or "optionalOkapiInterfaces" section of the package.json file. This is more actionable because the package.json file can be edited while the module descriptor doesn't exist in the code repository. A note should link to the package.json documentation: https://github.com/folio-org/stripes/blob/master/doc/dev-guide.md#the-package-file-stripes-entry

Frontend shared libraries should also contain complete "okapiInterfaces" or "optionalOkapiInterfaces" sections. The current criterium doesn't require this because a frontend shared library doesn't generate a module descriptor. For a complete okapi interface dependency documentation this change adds the requirement for frontend shared libraries.

The common criterium about interface requirements is split into frontend and backend criterium and moved from the Shared/Common section to the Frontend section and the Backend section.

This is the current acceptance criterium:

-* Module descriptor MUST include interface requirements for all consumed APIs (3, 5)
-  * _This is not applicable to libraries_

This should be more actionable.

Evaluators need to browse through the source code to find out the consumed APIs and then check that the API is listed.
Therefore the parts of the sentence should be swapped to start with "For each consumed API".

Evaluators of backend modules need to look up the interface requirement in the `"requires"` or `"optional"` section
of the module descriptor. The section names should be mentioned in the criterium to make it more actionable. The name
references the relevant part of the module descriptor documentation making the criterium easier to understand (the
preceding criterium links to the module descriptor documentation).
The criterium about the provided interfaces already mentions the `"provides"` section, similarly the the
`"requires"` and `"optional"` sections should be mentioned.

Currently the criterium about the interface requirements in the module descriptor also applies to frontend modules.
This can be simplified because the build process generates the module descriptor from the package.json file, and
there's already a criterium that it "MUST produce a valid module descriptor".

Therefore we can change the criterium for the frontend to require the interfaces of the consumed APIs to
be in the `"okapiInterfaces"` or `"optionalOkapiInterfaces"` section of the package.json file. This is more actionable
because the package.json file can be edited while the module descriptor doesn't exist in the code repository.
A note should link to the package.json documentation:
https://github.com/folio-org/stripes/blob/master/doc/dev-guide.md#the-package-file-stripes-entry

Frontend shared libraries should also contain complete `"okapiInterfaces"` or `"optionalOkapiInterfaces"` sections.
The current criterium doesn't require this because a frontend shared library doesn't generate a module descriptor.
For a complete okapi interface dependency documentation this change adds the requirement for frontend shared libraries.

The common criterium about interface requirements is split into frontend and backend criterium and moved from
the Shared/Common section to the Frontend section and the Backend section.
@julianladisch julianladisch requested a review from a team as a code owner September 9, 2024 16:58
@marcjohnson-kint
Copy link
Member

The section names should be mentioned in the criterium to make it more actionable

I agree with making the criteria more explicit and actionable

I don't understand why it is necessary to replace a single shared criteria in order to do that

This can be simplified because the build process generates the module descriptor from the package.json file

The criteria was deliberately written to avoid coupling it to mechanisms, as that makes the criteria brittle with respect to change of mechanisms rather than outputs / outcomes

I believe the criteria should minimise coupling to mechanisms as much as possible

there's already a criterium that it "MUST produce a valid module descriptor"

When the criteria was written, this only covered that the module descriptor was structurally / syntactically valid (in the sense that Okapi would accept it). It did not cover whether the descriptor is complete e.g. includes all consumed interfaces

@julianladisch
Copy link
Contributor Author

When we don't split the criterium into frontend and backend we get this entry in the Shared/Common section:

  • Module descriptor MUST include interface requirements for all consumed APIs (3, 5)
    • This is not applicable to backend libraries
    • For frontend modules and libraries
      • For each consumed API package.json MUST include the interface requirement in the "okapiInterfaces" or "optionalOkapiInterfaces" section (3, 5)
    • For backend modules
      • For each consumed API the module descriptor MUST include the interface requirement in the "requires" or "optional" section (3, 5)

This reduces readability for both developers and module evaluators.

@julianladisch julianladisch requested a review from a team November 19, 2024 09:18
@marcjohnson-kint
Copy link
Member

This reduces readability for both developers and module evaluators.

That is subjective, and it seems we disagree

Personally, I prefer a shared criteria that is generally applicable (and more detailed context specific advice, if helpful) over having multiple criteria that are more tightly coupled to specific details about that kind of thing being reviewed

@todolson
Copy link
Contributor

Agreed with expanding the criteria to be explicit about all consumed APIs.

Is it the case that backend libraries by definition cannot consume APIs? Or is it just that they don't have a module descriptor?

@maccabeelevine
Copy link
Member

I'm agnostic about most of this change but

there's already a criterium that it "MUST produce a valid module descriptor"

When the criteria was written, this only covered that the module descriptor was structurally / syntactically valid (in the sense that Okapi would accept it). It did not cover whether the descriptor is complete e.g. includes all consumed interfaces

I trust @marcjohnson-kint on the history here but it doesn't make much sense to me that this particular criteria @julianladisch is raising was about structure/syntax of the module descriptor, since as he says there is a separate criterion specifically about that.
And even if the original intent of this one was not to cover whether the descriptor is complete, I think that's a useful thing to check, and at least some of us have already been doing that in our evals.

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