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

review all TODOs #11

Closed
2 tasks done
yshavit opened this issue Jun 10, 2024 · 1 comment
Closed
2 tasks done

review all TODOs #11

yshavit opened this issue Jun 10, 2024 · 1 comment
Labels
task Internal task, like adding tests or doing refactorings.
Milestone

Comments

@yshavit
Copy link
Owner

yshavit commented Jun 10, 2024

  • Resolve each TODO, or file an Issue for it
  • Remove all #[allow(dead_code)]

I shouldn't do this until I get a bit more stable, though.

@yshavit yshavit added the task Internal task, like adding tests or doing refactorings. label Jun 10, 2024
@yshavit yshavit added this to the MVP 01 milestone Jun 24, 2024
yshavit added a commit that referenced this issue Jun 25, 2024
@yshavit
Copy link
Owner Author

yshavit commented Jul 16, 2024

Remove all #[allow(dead_code)]

It's already gone, and has been for a while.

yshavit added a commit that referenced this issue Jul 16, 2024
In service of #11.
yshavit added a commit that referenced this issue Jul 16, 2024
Default is `--output markdown`.

This way is more flexible in the future, if I want more output formats
for whatever reason.

Also fix an unrelated bug in the JSON output, which I found when I added
an integ test for it.

Resolves a TODO, in service of #11.
yshavit added a commit that referenced this issue Jul 17, 2024
Reference #84 in the code

In service of #11
yshavit added a commit that referenced this issue Jul 17, 2024
I have two spots in the code that I wasn't able to produce cases for. I
suspect these can't happen, but just in case, I'm going to put basically
an overridable panic of sorts. By default, hitting either of these cases
will result in an error; but if you provide the hidden
`--allow-unknown-markdown` flag, we'll just ignore them.

I can't test this (if I knew how to hit these branches, I wouldn't need
it!), but I manually added a `lookups.unknown_markdown("testing")?;` in
`MdElem::read` to verify that (a) it correctly errors out without the
flag, and (b) it correctly succeeds with the flag.

As part of this, I added better handling for the Err case of
`MdElem::read`.

Also, replace a TODO with a reference to #84.

This PR removes all the TODOs for #11. Once it gets merged, I'll check
the last checkbox on that ticket and manually close it.
@yshavit yshavit closed this as completed Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
task Internal task, like adding tests or doing refactorings.
Projects
None yet
Development

No branches or pull requests

1 participant