arm-learning-paths icon indicating copy to clipboard operation
arm-learning-paths copied to clipboard

Markdown code blocks without a type are allowed but can be misinterpreted

Open DavidSpickett opened this issue 2 years ago • 3 comments

Type of issue

  • [ ] Instructions in the learning path do not work.
  • [ ] Instructions in the learning path are hard to follow.
  • [ ] Spelling error in the learning path.
  • [x] Instructions to contribute a new learning path are not clear.
  • [ ] Other

Describe the issue

https://learn.arm.com/learning-paths/cross-platform/_example-learning-path/appendix-1-formatting/ says "You can add code snippets in the standard markdown format.".

Which is true, until you try to test a learning path and a single line block will fail because it has no commands. The first line is consumed as the "type" and then there are no lines left to be potential commands. For example:

```
Plain text goes here.
```

The type becomes "Plain text goes here." with no commands attached to the block. This leads to:

Traceback (most recent call last):
  File "./tools/maintenance.py", line 171, in <module>
    main()
  File "./tools/maintenance.py", line 148, in main
    check_lp(args.instructions, args.link, args.debug)
  File "./tools/maintenance.py", line 60, in check_lp
    res.append(check.check(lp_path + "/" + i[0], start=launch, stop=terminate))
  File "/home/davspi01/work/open_source/arm-learning-paths/tools/check.py", line 196, in check
    for j in range(0, t["ncmd"]):
KeyError: 'ncmd'

Which we could fix, but I think the real issue is we should clarify how blocks should or must be tagged with a type.

There are 2 workarounds to this first problem.

  1. Only have plain text that is at least 2 lines.
  2. Assign it some bogus type name or use text or output as the other learning paths have.

However, I think that's a poor user experience especially if you're using Markdown for its wide appeal.

If we want to require that all blocks have a type line, let's decide on a set of valid type names and throw a nice informative error if one of them is not found.

If we are not going to do that and instead allow the completely standard syntax, there is a gotcha that must be mentioned. If the first line of the plain text includes a recognised type like "bash", it will run the subsequent lines as bash. For example:

```
bash is cool
This is not a command.
```

Or even:

```
cool is bash
This is also not a command
```

Which I think would be incredibly confusing to a new contributor.

To Reproduce Create a learning path page with any of the blocks shown above, run the maintenance script on it.

Expected behavior Either:

  1. Completely standard markdown code blocks continue to be allowed, and we put in a fix for the lack of commands for single line blocks, and document the gotcha concerning the first line.
  2. All code blocks must have a type line at the start, and unknown types are raised to the user with a list of valid types they can choose from.
  3. Something else I haven't though of :)

Desktop (please complete the following information):

  • OS: Ubuntu Linux 20.04
  • Browser: chrome
  • Version: 119

Additional context Original "fix" for this was https://github.com/ArmDeveloperEcosystem/arm-learning-paths/pull/552 but this broke existing paths, so will be reverted.

DavidSpickett avatar Nov 02 '23 10:11 DavidSpickett

and we put in a fix for the lack of commands for single line blocks

One way is to initialise "ncmd" to 0 here

DavidSpickett avatar Nov 02 '23 10:11 DavidSpickett

Debug output for some of the examples:

```
bash is cool
start: 0x123 size: 345 allocated: true
```
[DEBUG]	Copying command to file .tmpcmd: start: 0x123 size: 345 allocated: true
[DEBUG]	['docker cp .tmpcmd test_0:/home/user/']
[DEBUG]	['docker exec -u user -w /home/user test_0 bash .tmpcmd']
[DEBUG]	Test 1: ERROR (command failed. Return code is 127 but expected 0)
```
cool is bash
start: 0x123 size: 345 allocated: true
```
[INFO]	Parsing content/learning-paths/laptops-and-desktops/dynamic-memory-allocator//2_designing_a_dynamic_memory_allocator.md
[DEBUG]	{'image': ['ubuntu:latest'], 'weight': -1}
[DEBUG]	{'type': 'C', 'ncmd': 2, 0: '#define STORAGE_SIZE 4096', 1: 'static char storage[STORAGE_SIZE];'}
[DEBUG]	{'type': 'bash', 'ret_code': '0', 'ncmd': 1, 0: 'start: 0x123 size: 345 allocated: true'}

So you can see the second line being used as the command.

DavidSpickett avatar Nov 02 '23 10:11 DavidSpickett

And to be clear, if the amount of info comes off as pushy this isn't my intention. I just wanted to give as clear a picture as possible.

I'm also happy to contribute the fixes myself, but being new here, I don't know what direction would be appropriate.

DavidSpickett avatar Nov 02 '23 10:11 DavidSpickett