Skip to content
This repository has been archived by the owner on Oct 15, 2024. It is now read-only.

Add example how to test rest api on localhost #2746

Closed
wants to merge 10 commits into from
Closed

Add example how to test rest api on localhost #2746

wants to merge 10 commits into from

Conversation

dmoisej
Copy link
Contributor

@dmoisej dmoisej commented Jun 4, 2019

Basics

Check relevant points but please do not remove entries.
Do not describe the purpose of this PR in the PR description but:

  • Short descriptions should be in the release notes (added as entry in
    doc/news/_preparation_next_release.md which contains _(my name)_)
    Please always add something to the the release notes.
  • Longer descriptions should be in documentation or in design decisions.
  • Describe details of how you changed the code in commit messages
    (first line should have module: short statement syntax)
  • References to issues, e.g. close #X, should be in the commit messages.

Checklist

Check relevant points but please do not remove entries.
For docu fixes, spell checking, and similar none of these points below
need to be checked.

  • I added unit tests
  • I ran all tests locally and everything went fine
  • affected documentation is fixed
  • I added code comments, logging, and assertions (see doc/CODING.md)
  • meta data is updated (e.g. README.md of plugins and doc/METADATA.md)

Review

Remove the line below and add the "work in progress" label if you do
not want the PR to be reviewed:

@markus2330 please review my pull request

Merging

Please add the "ready to merge" label when the build server and you say
that everything is ready to be merged.

@markus2330
Copy link
Contributor

Thank you very much for improving the docu!

Can you maybe use curl for the example, so that your examples can be actually executed?

@dmoisej
Copy link
Contributor Author

dmoisej commented Jun 4, 2019

@markus2330 yes, no problem, will add either today evening or tomorrow.

@dmoisej
Copy link
Contributor Author

dmoisej commented Jun 4, 2019

@markus2330 could you please help me and tell why some of the checks have failed? I just added changes to documentation and the new line to the changelog. Did I miss something?

@markus2330
Copy link
Contributor

The markdown formatting check fails, see: https://build.libelektra.org/jenkins/blue/organizations/jenkins/libelektra/detail/PR-2746/2/pipeline/223

@sanssecours is there really no way to turn off code-formatting inside of Markdown? The suggested change for the JSON is ugly.

@sanssecours
Copy link
Member

@sanssecours is there really no way to turn off code-formatting inside of Markdown?

You can disable the formatting using the tags <!-- prettier-ignore-start --> and <!-- prettier-ignore --> as specified in CODING.md.

The suggested change for the JSON is ugly.

I think the problem here is that prettier treats the JSON content as normal text and not as code, which is correct according to the Markdown specification. The HTML output below shows the rendering of the JSON content using GitHub’s Markdown converter.


{
"exists": true,
"name": "test",
"path": "user/test",
"ls": [
"user/test"
],
"value": "5",
"meta": ""
}


If you use a fenced code block, then prettier will format the code correctly and GitHub’s Markdown renderer will also produce nicer looking output.


{
  "exists": true,
  "name": "test",
  "path": "user/test",
  "ls": ["user/test"],
  "value": "5",
  "meta": ""
}

@markus2330
Copy link
Contributor

CODING.md does not mention that some code within Markdown also gets reformatted and it seems like that people do not expect this. Can you write the tip you gave here into CODING.md? (Btw. we should also consider to split up CODING.md. The instructions how to install the tools somehow do not belong there.)

@sanssecours
Copy link
Member

Can you write the tip you gave here into CODING.md?

The coding guidelines already specify that you should use fences for code blocks:

libelektra/doc/CODING.md

Lines 373 to 374 in a03fdfd

- Use [fences](https://help.github.com/en/articles/creating-and-highlighting-code-blocks) for code/examples
- Prefer fences which indicate the used language for better syntax highlighting

.

@markus2330
Copy link
Contributor

yes, but in which cases code might be reformatted is not explained. Or is it not reformatted in fences and fences is the only allowed option?

@sanssecours
Copy link
Member

yes, but in which cases code might be reformatted is not explained. Or is it not reformatted in fences and fences is the only allowed option?

As far as I can tell, prettier only changes code sections, if

  • you specify the language of a code block, and
  • it knows how to parse the specified language

.

@dmoisej
Copy link
Contributor Author

dmoisej commented Jun 5, 2019

@markus2330 I updated the example with using curl and I used fenced code block to show the result of the request.

@markus2330
Copy link
Contributor

@sanssecours can you maybe help out here?

curl http://localhost:33333/kdb/user/test
will return

```json
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 the exact output from curl? If it is, then please add the comment <!-- prettier-ignore-start --> before the code block and <!-- prettier-ignore-end --> after the code block. For an example, please take a look at the ReadMe of Yan LR:

<!-- prettier-ignore-start -->
```yaml
value
```
<!-- prettier-ignore-end -->

.

GET: http://localhost:33333/kdb/path

will return the value of path key, which is stored in the database.

Copy link
Member

Choose a reason for hiding this comment

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

I think part of an explanatory text is missing here. How about something like:

After you changed the value user/test with the command

kdb set user/test 5

, the command

curl http://localhost:33333/kdb/user/test

will return …

.

will return the value of path key, which is stored in the database.

```sh
kdb set /test 5
Copy link
Member

Choose a reason for hiding this comment

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

It might make sense to use user/test instead of /test here. This will make sure someone who reads this text does not have to know how cascading keys work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Once #2561 is fixed this should be identical anyway.

@dmoisej dmoisej mentioned this pull request Jun 10, 2019
14 tasks
@dmoisej dmoisej closed this Jun 10, 2019
@dmoisej
Copy link
Contributor Author

dmoisej commented Jun 10, 2019

Please see #2770

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

Successfully merging this pull request may close these issues.

3 participants