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

Readme defintion of each seems ot conflict with implementation #317

Merged
merged 1 commit into from
Oct 11, 2024

Conversation

stellarpower
Copy link
Contributor

The readme mentions that Node#each will iterate on a node and its first-level children (implying self will be yielded), but from the source file it seems that only the children are yielded.

Some basic testing seems to yield just the children for me, but feel free to correct me if I'm missing something here :)

The readme mentions that Node#each will iterate on a node and its first-level children (implying self will be yielded), but from [the source file](https://github.com/gjtorikian/commonmarker/blob/55c3b03708b31f1af37a53b3312f91400594223d/lib/commonmarker/node.rb#L27) it seems that only the children are yielded.
@gjtorikian
Copy link
Owner

Yes, you are correct. Thank you for the correction!

@gjtorikian gjtorikian merged commit 58d4c91 into gjtorikian:main Oct 11, 2024
1 check passed
@stellarpower
Copy link
Contributor Author

stellarpower commented Oct 11, 2024

Very welcome! A map might be useful too - but I guess that may have been left out intentionally to avoid ambiguity with walk.

Whilst I'm here actually, is there a quick way to list all the known node types? I see some inspect stuff but am not sure how to use it.

@gjtorikian
Copy link
Owner

I think the only for sure complete list is here:

"document" => ComrakNodeValue::Document,

It’s possible that the underlying comrak lib has more nodes that I haven’t yet ported over: https://github.com/kivikakk/comrak/blob/c36419327a09bfbe1326baf34db23f2c3f744f6f/src/nodes.rs#L15

@stellarpower
Copy link
Contributor Author

stellarpower commented Oct 11, 2024

Thanks, the rust docs in Comrak were what I'd been referring to. Somehow ruby is able to tab-complete some sensible options for the symbol, and as much as I like symbols, the disadvantage compared to a strictly-typed enum is it's hard to know all the options or what the right wording is. Yes python, I'm looking at you, with all those string keyword arguments rammed into one functions's signature that completely change the behaviour.

Maybe something to add (and that could be)? I was looking at the enum values, and given it's rust, I originally assumed that the enumeration was consumed with some boilerplate generator magic on the ruby side and the symbol was converted to it via string matching. But I guess from this you're calling to_s and then handling it on the rust side. So maybe not so trivial to manage then. I guess is it easy to keep those string constants in a list and return them?

@gjtorikian
Copy link
Owner

To be perfectly honest, my programming philosophy has always been:

  1. make it work
  2. make it good
  3. make it fast

The node AST iterating is somewhere close to 1 than 2. 😹 I needed it for work, so I threw whatever I could together to get the functionality. None of the design decision there had any deep thought past “just make it work.” But, if there are any features or changes you need that aren’t currently exposed, feel free to open an issue!

@stellarpower
Copy link
Contributor Author

Totally understand! I started out in C++, so I tend to iterate over that list in the exact opposite order - then eventually give up, duplicate my file, comment loads out, and go back in increasing order 😅. As long as you have all three in mind from the off I think it's fine.

I'll see if I can't add anything as I go. Likewise I just need this thing I'm working on sellotaped together and off my todo list ASAP. Cheers!

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

Successfully merging this pull request may close these issues.

2 participants