Skip to content
This repository has been archived by the owner on Jan 25, 2023. It is now read-only.

Markdown updates for 1.1.0 #279

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

Markdown updates for 1.1.0 #279

wants to merge 4 commits into from

Conversation

teemukataja
Copy link
Contributor

@teemukataja teemukataja commented Apr 30, 2019

As per request. Instead of copy-pasting from beacon.yaml I chose to update only the fields, and add new keys. Although, if you want consistency, I suggest writing examples to the former file and then copypasting the examples to the markdown file.

I strongly recommend abandoning double documentation, it is extra work to keep two separate documentation files in sync.

@teemukataja teemukataja requested a review from sdelatorrep April 30, 2019 05:16
Copy link
Contributor

@sdelatorrep sdelatorrep left a comment

Choose a reason for hiding this comment

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

Hi @teemukataja, good work but I spotted several things to fix:

  • the title still says v1.0.0
  • In "Beacon object", the example for apiVersion still says v1.0.0 (not sure if it's necessary to update it as it's just an example, though).
  • in BeaconAlleleResponse the field beaconHandover is missing.
  • in BeaconDatasetAlleleResponse there is a field beaconHandover but it should be datasetHandover.
  • the examples you pasted do not have these new fields beaconHandover or datasetHandover.
  • definition of ADA-M object has not been updated. In the table where adamDataUse is listed, we should add a link to the repository as we do with consentCodeDataUse field, and remove the section "Adam Data Use object".

@sdelatorrep
Copy link
Contributor

WDYT @juhtornr? Is there any reason to explain the fields, endpoints, parameters, etc, again in beacon.md? Because I agree with @teemukataja: it's annoying to maintain this file.

I suggest:

  • removing the description of the endpoints, parameters and objects as this information is already in the YAML file.
  • keeping the rest of the document (e.g. design principles, security, errors, ...) as this is not in the YAML
  • keeping the examples of requests and responses

@mbaudis
Copy link
Member

mbaudis commented May 2, 2019

Markdown from YAML?! We generate the whole set of SchemaBlocks examples and .md ... from the YAML file through a simple Perl script. E.g.

leads to

Sorry in case I misunderstood the problem ...

@sdelatorrep
Copy link
Contributor

This is very convenient for the .md files! (not so sure for the examples because we are currently showing real examples from a Beacon implementation, not theoretical examples)
We should definitely explore implementing this in next release (v1.1.1 or v1.2).

@mbaudis
Copy link
Member

mbaudis commented May 3, 2019

@sdelatorrep Is there a planned transition to JSON Schema? Since we'll do the same for SchemaBlocks (documentation from schema using some "standard tooling" - ask @relequestal... - && doc for website as now.

@juhtornr
Copy link
Collaborator

juhtornr commented May 8, 2019

WDYT @juhtornr? Is there any reason to explain the fields, endpoints, parameters, etc, again in beacon.md? Because I agree with @teemukataja: it's annoying to maintain this file.

I suggest:

  • removing the description of the endpoints, parameters and objects as this information is already in the YAML file.
  • keeping the rest of the document (e.g. design principles, security, errors, ...) as this is not in the YAML
  • keeping the examples of requests and responses

I think that it doesn't make sense to keep the same information in two different places. We should have just one place where the information is so therefore I agree with your proposal.

@teemukataja
Copy link
Contributor Author

Hi @teemukataja, good work but I spotted several things to fix:

* the title still says v1.0.0

Title now says v1.1.0

* In "Beacon object", the example for `apiVersion` still says `v1.0.0` (not sure if it's necessary to update it as it's just an example, though).

No longer applicable, as objects were removed.

* in `BeaconAlleleResponse` the field `beaconHandover` is missing.

Added.

* in `BeaconDatasetAlleleResponse` there is a field `beaconHandover` but it should be `datasetHandover`.

No longer applicable, as objects were removed.

* the examples you pasted do not have these new fields `beaconHandover` or `datasetHandover`.

Added missing fields.

* definition of ADA-M object has not been updated. In the table where `adamDataUse` is listed, we should add a link to the [repository](https://raw.githubusercontent.com/ga4gh/ADA-M/v1.0.1/adam.yaml) as we do with `consentCodeDataUse` field, and remove the section "Adam Data Use object".

No longer applicable, as objects were removed.

WDYT @juhtornr? Is there any reason to explain the fields, endpoints, parameters, etc, again in beacon.md? Because I agree with @teemukataja: it's annoying to maintain this file.

I suggest:

* removing the description of the endpoints, parameters and objects as this information is already in the YAML file.

Removed

* keeping the rest of the document (e.g. design principles, security, errors, ...) as this is not in the YAML

Kept these.

* keeping the examples of requests and responses

Kept these. (Although Swagger displays responses as well, but not the requests. Would be nice to remove these as well and only keep example requests maybe?)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants