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

Add location information to as many elements as possible #166

Closed
mivok opened this issue Sep 6, 2014 · 7 comments
Closed

Add location information to as many elements as possible #166

mivok opened this issue Sep 6, 2014 · 7 comments
Assignees
Labels

Comments

@mivok
Copy link

@mivok mivok commented Sep 6, 2014

#96 (the original pull request for adding location information) makes mention of the fact that location information isn't in all elements such as text elements or blank lines, with the reasons being given as 'too much noise'. There are some situations where it would be nice to identify line information even in these elements (in my case, I want to scan for all text elements whose contents match a certain regular expression and print their line numbers), and I'm seeing commits (e.g. 16b5524) that add in support for location information in more elements. What are your thoughts on adding location information to all element types?

@gettalong gettalong self-assigned this Sep 7, 2014
@gettalong
Copy link
Owner

@gettalong gettalong commented Sep 7, 2014

I haven't really thought about it, I have to admit.

@jhund What do you think?

@jhund
Copy link
Contributor

@jhund jhund commented Sep 8, 2014

We initially added location information only to those elements we needed it for (in repositext). The reasons for not adding it to all elements were these:

  • would have required more time to implement
  • impact on performance
  • additional noise when inspecting elements (a lot of elements that wouldn't print the options hash now would because of line information).

So there are tradeoffs to adding location information to all elements, however it may be well worth the cost to add it to all elements. It sure helps with debugging and inspecting kramdown documents.

I found that I didn't need to compute location information for text elements. It was sufficient to use the parent block element's location. Maybe we could avoid further performance impact by using the parent element's line number for text elements (with a potential loss in precision).

@gettalong
Copy link
Owner

@gettalong gettalong commented Sep 9, 2014

I did a simple benchmark (converting a 1MB document) and it ran in about the same time even after adding the location information to the :text elements.

So I will go forward and implement this, keeping in mind possible performance problems.

@gettalong
Copy link
Owner

@gettalong gettalong commented Sep 13, 2014

@jhund Thanks for the information regarding text elements -- this is how it is actually done now because it was the solution with the least changes 😄

@gettalong
Copy link
Owner

@gettalong gettalong commented Sep 13, 2014

@jhund @mivok So, after adding location information to more elements the performance seems to be about the same. In the big test case (1MB document) there doesn't seem to be any slowdown.

I have pushed the latest changes to Github, could you try it out and tell me if this works for you? I would like to release a new version on Monday, now that the issue is solved.

@mivok
Copy link
Author

@mivok mivok commented Sep 13, 2014

This looks awesome. Thanks! It fixes the issue I have currently.

I do still see missing location info on blank elements (e.g. newlines), but I've not hit a situation (yet) where I need that.

I'm looking forward to the new release.

:shipit:

@gettalong gettalong closed this Sep 16, 2014
@gettalong
Copy link
Owner

@gettalong gettalong commented Sep 16, 2014

@jhund @mivok Just a heads up: There was a major performance regression regarding the location reporting since 1.4.0 which I have also fixed now. So larger documents should now be parsed/converted as fast as before the regression

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants