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

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

Open
1 of 5 tasks
DavidSpickett opened this issue Nov 2, 2023 · 3 comments
Open
1 of 5 tasks

Comments

@DavidSpickett
Copy link
Contributor

DavidSpickett commented Nov 2, 2023

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.
  • 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 #552 but this broke existing paths, so will be reverted.

DavidSpickett added a commit to DavidSpickett/arm-learning-paths that referenced this issue Nov 2, 2023
This reverts commit abbb940.

Due to causing existing learning paths to fail tests. I've opened
issue ArmDeveloperEcosystem#562 to discuss the larger issue here, which I did not understand
when I did this change.
@DavidSpickett
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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 DavidSpickett changed the title Markdown code blocks without a type are allowed but can easily be misinterpreted Markdown code blocks without a type are allowed but can be misinterpreted Nov 2, 2023
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

No branches or pull requests

1 participant