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

Multi-line comments etc. #2

Closed
wants to merge 5 commits into from
Closed

Conversation

facefunk
Copy link

Hello,

I've incorporated comment parsing, both single and multi-line directly into the CodeTree parser, while this does reduce the agnosticism of the CodeTree format a bit I believed it was the optimal place to do it. I'm also allowing multi-file input and recording the type, line number and filename of each node with a view to generating CSS sourcemaps from Scarlet.

Possibly contentious: I've changed the main parsing loop to a finite(ish) state machine reading a byte at a time from a Reader. I think using a Reader is more idiomatic, versatile and, assuming the file is re-read every time which in the wild it would be, optimal as there's less copying going on. See commit for benchmarks. However this may be undesirable if it begins to carry the project away from an ideal of beautiful simplicity that you've certainly achieved, if so I understand completely.

CodeTree.go Outdated Show resolved Hide resolved
@codecov-io
Copy link

Codecov Report

Merging #2 into master will decrease coverage by 20.19%.
The diff coverage is 70.83%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #2      +/-   ##
==========================================
- Coverage   94.73%   74.54%   -20.2%     
==========================================
  Files           1        1              
  Lines          57      220     +163     
==========================================
+ Hits           54      164     +110     
- Misses          1       43      +42     
- Partials        2       13      +11
Impacted Files Coverage Δ
CodeTree.go 74.54% <70.83%> (-20.2%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 56da70d...51d177e. Read the comment docs.

@akyoto
Copy link
Member

akyoto commented May 1, 2019

The bad:

  • Dave Cheney's Talk, then code explains why it is important to contact the author before starting a change. I would have preferred it if you contacted me via email, Discord or GitHub issues.
  • The 100% coverage decreases by 26% if I can trust the Codecov automation. This is bad because I'm currently putting in an effort to increase the coverage in multiple projects. If you could provide the same or almost the same coverage, it would ease my task of judging a PR.
  • Multi-line comments increase the complexity of not only this tool but also every other tool that will be developed for supported languages in the future. To decrease tooling complexity, I believe this is a feature that doesn't belong into this repository because CodeTree is not concerned about the contents of the tree. I would generally advise to stay away from multi-line comments.
  • The style doesn't meet the style guidelines. I will try to include this link in my packages in the future.

The good:

  • Using an io.Reader instead of a string. This is a very good change, performance- and style-wise.

Conclusion:

What I can offer is to implement the best part of this PR which imho is the interface change from a string to an io.Reader. I fully agree with you on that part.

@facefunk
Copy link
Author

facefunk commented May 1, 2019

Of course, I completely understand. I had already made these changes and thought I'd run them past you, it didn't occur to me that a pull request might not be the best place to do this. Thanks very much for taking the time to review and respond with such useful information.

@facefunk facefunk closed this May 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants