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

handle markdown files #37

Closed
drahnr opened this issue Jun 18, 2020 · 10 comments · Fixed by #74
Closed

handle markdown files #37

drahnr opened this issue Jun 18, 2020 · 10 comments · Fixed by #74
Assignees
Labels
enhancement 🦚 New feature or request good first issue 🔰 Good for newcomers
Projects
Milestone

Comments

@drahnr
Copy link
Owner

drahnr commented Jun 18, 2020

Summary

Enable handling of *.md files, the infrastructure is already there and the handling that is necessary is mostly in traverse.rs and in documentation.rs

@drahnr drahnr added enhancement 🦚 New feature or request good first issue 🔰 Good for newcomers labels Jun 18, 2020
@drahnr drahnr mentioned this issue Jun 19, 2020
3 tasks
@zhiburt
Copy link
Contributor

zhiburt commented Jun 21, 2020

Hey @drahnr, I investigated this issue a bit and as I see there are a bunch of todos in the code in regard of this issues. Which is really helpful.

Here's one comment which I'd love to have a cast light on :)

pub struct Documentation {
/// Mapping of a path to documentation literals
// @todo add an intermediate enum to be able to handle markdown files as part of a document too
index: IndexMap<PathBuf, Vec<LiteralSet>>,
}

It's no clear to me if you want reuseVec<TrimmedLiteral> and create just a marker of a type of documentation it holds.

Mainly as I see to use the current structure the work should be piled on existent types such as TrimmedLiteral. But TrimmedLiteral cann't be build from markdown filles (syn::parse_str fails parsing it).

Firstly I tried to extract TrimmedLiteral with removed corresponding field and create additional type RustTrimmedLiteral which has this field . The idea was that markdown files produces TrimmedLiteral but rust files RustTrimmedLiteral. Eventually I hope there's another way here as this is far too complicated I would expect here, and that's why I am here :)

And as I understand for getting literals out of markdown there must be used a PlainOverlay type, and specifically mapping field right?


Some thoughts out of my mind as I kinda stuck with that 😞

@drahnr
Copy link
Owner Author

drahnr commented Jun 21, 2020

I'll write smth up in a few minutes - I have an idea how to solve this cleanly and also help making #18 testable as a sideeffect :)


So the idea here is to make the the Documentation independent of TrimmedLiteralSet, but use a struct containing a String and Range -> Span mapping of the original file.
Note that this is non-trivial work, a few places need refactoring and calculation.

The first step is to introduce an enum like:

#[derive(Debug,Clone,Hash,Eq,PartialEq)]
enum ContentSource {
CommonMarkFile(PathBuf),
RustDocTest(PathBuf, Span), // span is just there to disambiguiate
RustSourceFile(PathBuf),
}

and use it with

 pub struct Documentation { 
     index: IndexMap<ContentSource, Vec<CheckableChunk>>, 
 } 

where CheckableChunk

struct CheckableChunk {
content: String,
source_lookup: IndexMap<Range,Span>
}

(CC @KuabeM which is what I meant when I said, decouple from proc_macro2::{Span,Literal}

which is extracted from TrimmedLiteralSet/LiteralSet. Anything later on should only work on CheckableChunks.

Does that make sense to you @zhiburt ?

Note that I significantly underestimated the changes needed for this, so I am going to remove the good first issue tag, let me know if you still want to tackle it :)

@drahnr drahnr added heavy-duty 🚜 Big features not easy to implement and removed good first issue 🔰 Good for newcomers labels Jun 21, 2020
@drahnr
Copy link
Owner Author

drahnr commented Jun 22, 2020

Please see #55 which includes most of what is needed :) feel free to pick it up from there, I left @todo where things are left to do

@drahnr drahnr added good first issue 🔰 Good for newcomers and removed heavy-duty 🚜 Big features not easy to implement labels Jul 2, 2020
@drahnr
Copy link
Owner Author

drahnr commented Jul 2, 2020

@zhiburt #55 is almost done, given that, the fundamental structure is already there :)

@drahnr drahnr added this to To Do in Overview Jul 8, 2020
@drahnr
Copy link
Owner Author

drahnr commented Jul 10, 2020

So #55 is complete and merged, feel free to take a shot or assign the issue back to me and I'll take over :)

@zhiburt
Copy link
Contributor

zhiburt commented Jul 10, 2020

😄 I actually took a shot ones to implement it but didn't got too far 😞
Sorry for my delays.

I'll try to put it in action one more and tell how it will be.

@zhiburt
Copy link
Contributor

zhiburt commented Jul 12, 2020

Got into frustration one more time 😄
I see that probably an only missed part is

let source_mapping = IndexMap::new(); // @todo source map should be trivial, start to end

Adding an item such as below to the map allows to see the .md files in the output but awkwardly.

                        let mut source_mapping = IndexMap::new();
                        source_mapping.insert(
                            0..content.len(),
                            Span {
                                start: proc_macro2::LineColumn { line: 0, column: 0 },
                                end: proc_macro2::LineColumn { line: 0, column: 0 },
                            },
                        );

So I can't figure out how to put it on the rails properly yet :(
I you could give any clue would be helpful.

@drahnr
Copy link
Owner Author

drahnr commented Jul 12, 2020

That would be roughly my approach too to just use a range / span covering the full markdown file.

Did you fork the repo again? Lots changed and the proc_macro2::Span should not be used outside of CheckableChunk construction anymore.

Could you create a PR and add some comments on what particular thing / section you do not understand plus your current attempted changes, that would allow me to give you better directions :)

@drahnr drahnr added this to the v0.3.0 milestone Jul 14, 2020
@zhiburt
Copy link
Contributor

zhiburt commented Jul 16, 2020

It's not working properly 😞 but here it is

e45125d

@drahnr
Copy link
Owner Author

drahnr commented Jul 17, 2020

You got pretty close, I wasn't sure so I implemented the surface part (almost the same as you did, take a look at e535f38 ) which is soon going to hit master soon - but I this was just the first step and there were plenty of other issues with that and actually required #25 to almost implemented - which was a bit of a heavy weight

Don't get discouraged, I'll try to add some conceptual overview graphics so it's easier to understand the individual components better :)

Overview automation moved this from To Do to Done Jul 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 🦚 New feature or request good first issue 🔰 Good for newcomers
Projects
Overview
  
Done
Development

Successfully merging a pull request may close this issue.

2 participants