Check markdown rs #5621

Christopher-C-Robinson · 2023-04-25T15:38:26Z

Draft of markdown parser

Signed-off-by: Domesticcadiz <[email protected]>

…st can see this commit! Signed-off-by: Max Wendlandt <[email protected]>

Signed-off-by: Domesticcadiz <[email protected]>

jodh-intel · 2023-04-25T16:21:10Z

Speaking to @Christopher-C-Robinson and @Toreseen, we know that the following needs to be done to this draft PR:

Remove the target/ binaries introduced by the editor we believe.
Remove the src/main_from_go.rs and src/validate_structure.rs.
Possibly split the functionality out of src/main.rs into one or more other files below src/.
Rewrite the README.md to contain details of the tool itself (it's currently a copy of the top-level README.md).
Squash all the commits down into one.
Update the commit format to match https://github.com/kata-containers/community/blob/main/CONTRIBUTING.md#patch-format.

jodh-intel

Thanks @Christopher-C-Robinson and @Toreseen - this is looking good! I've added a few comments.

More generally, I know we've discussed this. But looking at the code I do think it might make sense to split the main into a few other files as this grows. Maybe:

src/output.rs
src/node.rs
src/toc.rs
src/stats.rs

... and then add a corresponding mod output, mod node, etc in src/main.rs to "activate" these modules.

Also, it would be good to add a few unit tests as that will help shake out bugs and increase your confidence in the functions as you'll know they can tolerate all types of nasty input (writing tests can be fun as you try to find lots of devious inputs for your functions to handle 😈)

jodh-intel · 2023-04-26T07:48:28Z

cmd/check-markdown-rs/src/main.rs

+    let input = fs::read_to_string(input_file_path)?;
+    let root = parse_document(&arena, &input, &ComrakOptions::default());
+
+    let toc = generate_toc(&root, &arena);


It would be good to only display this when the toc subcommand (or an option) is specified. We use a sub-command for the existing golang tool:

https://github.com/kata-containers/tests/blob/main/cmd/check-markdown/main.go#L302

You'll want to cargo add clap and then ideally create structs and/or enums to represent, declaratively, the CLI options. For examples, see:

src/args.rs on https://github.com/kata-containers/kata-containers/pull/6512/files#diff-31f90316d2fc8a73eb61565194116992ec5a56ab4305a44496a35a0e3327a0da

https://github.com/kata-containers/kata-containers/blob/main/src/tools/kata-ctl/src/args.rs

jodh-intel · 2023-04-26T07:56:22Z

cmd/check-markdown-rs/src/main.rs

+        eprintln!("Usage: {} <input_file>", args[0]);
+    }
+
+    let input_file_path = env::current_dir()?.join("src").join(args[1].clone());


This looks a bit odd: why do we need to add src? Although it might work with cargo run -- $file, will it also work if you cargo install and then run check-markdown-rs "$file"?

Im not sure if this is what you are lookin for, bit I'm pushing a commit now that removes the SRC from the path, assuming that you will already cd into the src directory.

jodh-intel · 2023-04-26T08:02:49Z

cmd/check-markdown-rs/src/main.rs

+        NodeValue::Heading(heading) => println!(
+            "{}Header (level {}): {}",
+            indent,
+            heading.level,
+            get_node_text(&node)
+        ),


I know these blocks are small, but I'd be inclined to create a set of functions to handle each node type:

handle_heading_node(indent, level, text)

handle_text_node(indent, text)

handle_link_node(indent, title, url)

The advantages of doing this include:

simplifies print_nodes() and makes it easier to read.

makes it easier to unit test different types of nodes raising confidence in the parts of the code you have tested formally.

I think I added what you were looking for.

jodh-intel · 2023-04-26T08:03:35Z

cmd/check-markdown-rs/src/main.rs

+                        return Err(String::from("Error: More than one H1 heading found."));
+                    }
+                }
+
+                if level > last_seen_level + 1 {
+                    return Err(String::from("Error: Invalid heading level order."));


Can you remove the Error: prefix from the errors? The type of these objects Result::Err already encodes that they are errors so it's best to let the higher levels handle adding prefixes like this (see comment on that topic below).

Can you include the heading in the error being returned here? In fact, ideally, you'd also add the line number and the filename to the error (either here or at a higher level) to make it 100% clear to the user which part of which file contains the error. This is particularly important when the tool starts to jump between files when it finds a cross-file link, and then finds, for example, an invalid link in another file with the same name as a link in the initial file (yep - I've been there and done that ;)

The first part of this should be fixed now!

jodh-intel · 2023-04-26T08:05:43Z

cmd/check-markdown-rs/src/main.rs

+            println!("  Found {} invalid links:", errors.len());
+            for error in errors {
+                println!("    {}", error);
+            }


I like the way you've collected all the errors together here and are displaying them at the end. Conventionally, errors should be displayed to standard error stream, so can you change the println!() calls to eprintln!() and add an "ERROR: " prefix to the call in the for loop?

Signed-off-by: Max Wendlandt <[email protected]>

Unneeded file

Signed-off-by: Max Wendlandt <[email protected]>

Signed-off-by: Maxwell Wendlandt <[email protected]>

Signed-off-by: Domesticcadiz <[email protected]>

Signed-off-by: Maxwell Wendlandt <[email protected]>

jodh-intel · 2023-05-02T12:48:06Z

cmd/check-markdown-rs/README.md

+cargo build --release
+./target/release/markdown_parser input_file.md


Nit: It might be safe to install the command to avoid the target line potentially being incorrect:

$ cargo install --path . $ check-markdown-rs input_file.md

sounds good!

jodh-intel · 2023-05-02T12:50:43Z

cmd/check-markdown-rs/src/main.rs

+    if args.len() < 2 {
+        eprintln!("Usage: {} <input_file>", args[0]);
+    }


You need to call std::process::exit(1) after displaying the message: currently, the message is displayed but the program keeps running, then fails:
$ check-markdown-rs Usage: check-markdown-rs <input_file> thread 'main' panicked at 'index out of bounds: the len is 1 but the index is 1', src/main.rs:22:52 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Please can you add some argument parsing code so that the user can run:
$ check-markdown-rs --help $ check-markdown-rs --version

…curred in the markdown doc Signed-off-by: Maxwell Wendlandt <[email protected]>

jodh-intel

Hi @Christopher-C-Robinson and @Toreseen,

As we're very close to the end of the outreach project now, I suggest you rework this PR to remove the binary files, etc as follows. Note that once you've done this though, @Toreseen won't be able to git pull from @Christopher-C-Robinson's fork, so you'll both have to work together and have just @Christopher-C-Robinson git push -f to his fork once you've followed the steps below. The steps below are similar in principle to what we did in the meeting on Tuesday with Alec and Jenny's branch. Yes, this is "hacky" in the sense that we're not using git to resolve the problems - we're simply recreating the branch, but I think this approach is simpler to understand. As always, make sure you have a copy of your local branch/repo before doing the following:

# Update your copy of the main branch
git checkout main
git pull

# Switch to your feature branch
git checkout check-markdown-rs

# Save the files you've created for this new tool
cp cmd/check-markdown-rs/Cargo.toml cmd/check-markdown-rs/README.md cmd/check-markdown-rs/src/main.rs /tmp/

# Rename the original version of the branch (it's now a "backup")
git branch -m check-markdown-rs-original

# Create a new clean version of the feature branch
git checkout main
git checkout -b check-markdown-rs

# Recreate the branch
mkdir cmd/check-markdown-rs
cd cmd/check-markdown-rs
cp /tmp/Cargo.toml /tmp/README.md .
mkdir src
cp /tmp/main.rs src

# Add the new files to git
git add Cargo.toml README.md src/main.rs

# Save the changes
git commit -as

# Update your fork (which will automatically update this PR)
git push -f

Once you've done that, you'll also need to update cmd/README.md to add an entry for your new tool, so:

Add an entry for your new tool to cmd/README.md
git commit -as
Merge the new commit with the main commit by running git rebase -i main.
Change the 2nd commit to squash.
Save and quit the editor.
Update the PR again by running, git push -f

jodh-intel · 2023-05-04T09:44:19Z

cmd/README.md

@@ -1,13 +0,0 @@
-# CLI tools


Hi @Christopher-C-Robinson and @Toreseen - I don't think this file should have been deleted?

The instructions above show you how to recreate the entire branch which will resolve this issue 😄

I see that now. I added that back in. should be pushed sometime tonight.

Christopher-C-Robinson · 2023-05-04T10:13:31Z

@jodh-intel

@Toreseen and I have done some serious work tonight on our code. If you get a chance, can you take a look at it. Might be messy bit it works. We both spent about 12 hours each on it individually. I’m finally off to bed. Hopefully we are getting close!

Signed-off-by: Maxwell Wendlandt <[email protected]>

… function. Signed-off-by: Maxwell Wendlandt <[email protected]>

jodh-intel

Thanks @Christopher-C-Robinson and @Toreseen ! This is very close now.

Before we can get this landed though, you'll need to clean up the branch to (a) remove all the binary files; and (b) collapse all your commits into a single one - see my big "branch recreation" comment.

jodh-intel · 2023-05-05T08:53:26Z

cmd/check-markdown-rs/src/main.rs

+        eprintln!("Usage: {} <input_file>", args[0]);
+        return Ok(());


Nit: Ideally, this would return an Err as this is it invalid to call the tool without specifying a file.

i think this is fixed now

Christopher-C-Robinson · 2023-05-08T12:02:50Z

Thanks @Christopher-C-Robinson and @Toreseen - this is looking good! I've added a few comments.

More generally, I know we've discussed this. But looking at the code I do think it might make sense to split the main into a few other files as this grows. Maybe:
src/output.rs
src/node.rs
src/toc.rs
src/stats.rs
... and then add a corresponding mod output, mod node, etc in src/main.rs to "activate" these modules.

Also, it would be good to add a few unit tests as that will help shake out bugs and increase your confidence in the functions as you'll know they can tolerate all types of nasty input (writing tests can be fun as you try to find lots of devious inputs for your functions to handle 😈)

@jodh-intel , I split the main.rs into different files. let me know what your thoughts are. I will work more on it tonight.

Signed-off-by: Maxwell Wendlandt <[email protected]>

…ther requested freatures. Signed-off-by: Maxwell Wendlandt <[email protected]>

jodh-intel

Thanks @Christopher-C-Robinson and @Toreseen - this is looking good.

Could you remove the binary files from this PR now? Also, could you delete the unused functions that cargo build lists?

Christopher-C-Robinson and others added 4 commits April 14, 2023 09:50

added file

b35a27e

Signed-off-by: Domesticcadiz <[email protected]>

skeleton files

decf4ae

Signed-off-by: Domesticcadiz <[email protected]>

Commiting validate_document_structure.rs. Also testing to see if Chri…

9553168

…st can see this commit! Signed-off-by: Max Wendlandt <[email protected]>

latest code

8cd5a5b

Signed-off-by: Domesticcadiz <[email protected]>

Christopher-C-Robinson added the wip Work in Progress (PR incomplete - needs more work or rework) label Apr 25, 2023

Christopher-C-Robinson requested a review from a team as a code owner April 25, 2023 15:38

katacontainersbot added the size/tiny Smallest and simplest task label Apr 25, 2023

removed binary files

821dc04

Signed-off-by: Domesticcadiz <[email protected]>

jodh-intel reviewed Apr 26, 2023

View reviewed changes

Toreseen added 3 commits April 26, 2023 14:22

Commiting small changes for Chris to get on his laptop

0d52f30

Signed-off-by: Max Wendlandt <[email protected]>

Delete main_from_go.rs

4056bd1

Unneeded file

Delete validate_structure.rs

cd9b534

Unneeded file

katacontainersbot added size/huge Largest and most complex task (probably needs breaking into small pieces) and removed size/tiny Smallest and simplest task labels Apr 26, 2023

Toreseen and others added 6 commits April 26, 2023 14:52

Commit attempt number 14. Lets see how this goes.

54ecb84

Signed-off-by: Max Wendlandt <[email protected]>

Commit number 2, sort of another test to see if this works

5ddca97

Signed-off-by: Max Wendlandt <[email protected]>

Delete .rustc_info.json

730fab7

Delete CACHEDIR.TAG

e35c532

Removed the ERROR prefix from the error messages.

e9a308e

Signed-off-by: Maxwell Wendlandt <[email protected]>

updated readme files

4fd4ba3

Signed-off-by: Domesticcadiz <[email protected]>

katacontainersbot added size/tiny Smallest and simplest task and removed size/huge Largest and most complex task (probably needs breaking into small pieces) labels Apr 30, 2023

Toreseen and others added 2 commits April 30, 2023 17:49

Fixed SRC being using in input_file_path.

efe9d3e

Signed-off-by: Maxwell Wendlandt <[email protected]>

updated readme

853b8c0

katacontainersbot added size/huge Largest and most complex task (probably needs breaking into small pieces) and removed size/tiny Smallest and simplest task labels Apr 30, 2023

jodh-intel reviewed May 2, 2023

View reviewed changes

Added line number's to be output in error messages where the error oc…

66a65a5

…curred in the markdown doc Signed-off-by: Maxwell Wendlandt <[email protected]>

jodh-intel reviewed May 4, 2023

View reviewed changes

updated to validate links

eaedaea

Toreseen and others added 3 commits May 4, 2023 23:09

Fixed one small indentatio error

7f1591e

Signed-off-by: Maxwell Wendlandt <[email protected]>

added helper functions. fixed deleted readme.md

aad2762

Added heading titles and file names to the document heaing validation…

d6eab95

… function. Signed-off-by: Maxwell Wendlandt <[email protected]>

katacontainersbot added size/tiny Smallest and simplest task and removed size/huge Largest and most complex task (probably needs breaking into small pieces) labels May 5, 2023

jodh-intel reviewed May 5, 2023

View reviewed changes

added comments. fixed one of james's comments

c14745b

katacontainersbot added size/huge Largest and most complex task (probably needs breaking into small pieces) and removed size/tiny Smallest and simplest task labels May 8, 2023

split into multiple files

9f65c40

Christopher-C-Robinson and others added 2 commits May 8, 2023 07:28

install command avoid target line being incorrect

8fbc30f

Added copyright (c) to rust files

3754706

Signed-off-by: Maxwell Wendlandt <[email protected]>

katacontainersbot added size/tiny Smallest and simplest task and removed size/huge Largest and most complex task (probably needs breaking into small pieces) labels May 8, 2023

Added the functionaity for H1 heading errors to ouput line number + o…

b0736e2

…ther requested freatures. Signed-off-by: Maxwell Wendlandt <[email protected]>

katacontainersbot added size/huge Largest and most complex task (probably needs breaking into small pieces) and removed size/tiny Smallest and simplest task labels May 9, 2023

jodh-intel reviewed May 9, 2023

View reviewed changes

cleaned up unused methads and variables

f53ee46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check markdown rs #5621

Check markdown rs #5621

Christopher-C-Robinson commented Apr 25, 2023

jodh-intel commented Apr 25, 2023 •

edited by Christopher-C-Robinson

Loading

jodh-intel left a comment

jodh-intel Apr 26, 2023

jodh-intel Apr 26, 2023 •

edited

Loading

Toreseen Apr 30, 2023

jodh-intel Apr 26, 2023

Christopher-C-Robinson May 5, 2023

jodh-intel Apr 26, 2023

Toreseen Apr 30, 2023 •

edited

Loading

jodh-intel Apr 26, 2023

jodh-intel May 2, 2023

Christopher-C-Robinson May 8, 2023

jodh-intel May 2, 2023

jodh-intel left a comment

jodh-intel May 4, 2023

jodh-intel May 4, 2023

Christopher-C-Robinson May 5, 2023

Christopher-C-Robinson commented May 4, 2023

jodh-intel left a comment

jodh-intel May 5, 2023

Christopher-C-Robinson May 8, 2023

Christopher-C-Robinson commented May 8, 2023

jodh-intel left a comment

		cargo build --release
		./target/release/markdown_parser input_file.md

Check markdown rs #5621

Are you sure you want to change the base?

Check markdown rs #5621

Conversation

Christopher-C-Robinson commented Apr 25, 2023

jodh-intel commented Apr 25, 2023 • edited by Christopher-C-Robinson Loading

jodh-intel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jodh-intel Apr 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Toreseen Apr 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jodh-intel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Christopher-C-Robinson commented May 4, 2023

jodh-intel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Christopher-C-Robinson commented May 8, 2023

jodh-intel left a comment

Choose a reason for hiding this comment

jodh-intel commented Apr 25, 2023 •

edited by Christopher-C-Robinson

Loading

jodh-intel Apr 26, 2023 •

edited

Loading

Toreseen Apr 30, 2023 •

edited

Loading