Implement Spans via generics #14
Conversation
There was a problem hiding this comment.
I am curious to benchmark this (specifically without spans support) -- i should build hyperlink against this sometime and run it against some larger static site to see what happens. I should do that next (but can't promise when, sorry)
I'm also really unsure about the changes to emitter trait, but other than that, good stuff
| @@ -53,13 +54,13 @@ pub trait Emitter { | |||
| fn emit_string(&mut self, c: &str); | |||
|
|
|||
| /// Set the _current token_ to a start tag. | |||
| fn init_start_tag(&mut self); | |||
| fn init_start_tag(&mut self, reader: &R); | |||
There was a problem hiding this comment.
Adding reader everywhere is extremely unfortunate. Let's say some implementor of emit_string wants a Reader at some point: it would be a breaking change to add it to the method signature
also this change by itself is a breaking change
not entirely sure what to do about it, need more time to decide
There was a problem hiding this comment.
Adding reader everywhere is extremely unfortunate.
I found it a quite elegant solution because it keeps the Emitter trait unaware of spans.
Let's say some implementor of emit_string wants a Reader at some point: it would be a breaking change to add it to the method signature
If you want to cater to unforeseen future use cases we could add the reader: &R parameter to all Emitter methods.
also this change by itself is a breaking change
Yes I know. But the crate is still 0.2, so I don't think it should be that big of a deal? You said yourself that your API is still "very in-flux".
There was a problem hiding this comment.
yes just thinking out loud, no better solution in sight
There was a problem hiding this comment.
If you want to cater to unforeseen future use cases we could add the reader: &R parameter to all Emitter methods.
I think that's probably good, yeah
|
Of course :) If you decide to merge this please don't squash it ... I worked hard on producing a readable git history (same for my other PR). |
|
Sorry I can't do that. I want to keep the commit log short and readable, and to me the logical change is that you added spans support here. The way I like to operate on this project is:
This is how I operate all my repos nowadays. It's the least amount of headache, makes everybody's contributions (which have varying quality in commit messages) look the same, and ensures jumping from commit to PR (and then to issue) actually works. You may argue that a lot of context is lost in GitHub then, but that's already the case. All the discussions and reviews need to be preserved somehow else too. I apprechiate your effort nonetheless, it's just that I don't want to change how I run oss projects. |
|
I appreciate your point of view. It's just that I don't want to change how I contribute to oss projects. I think I would honestly rather fork your project than have you squash these commits. Yes, I have noticed that GitHub has introduced the trend of "just squashing everything". Although every time I have brought up that I prefer my commits to be merged unsquashed maintainers have been happy to honor my wishes. This is the first time I am confronted with such disrespect and I find it seriously upsetting. |
|
If you'd not like to contribute on the terms that this project is being run on, that is fine, but I don't think that anybody is limiting you or demanding you change how you contribute by having a uniform policy on how commits look like in their project. I don't think I am being disrespectful to you or the work you do. I didn't ask you to change how you commit within your branch. You're asking me to change how I commit to my project. |
|
Right, I see that you don't understand my point of view so let me clarify. I absolutely do not care about my branch. When a branch of mine is merged ... I delete it. What I strongly care about is producing a readable git history. Readable in the sense that others can easily understand how larger changes came to be. I was merely asking you not to throw away the effort I put into this by squashing my commits. I am asking you to not change my commits ... because squashing is actively changing them (even if that is just a click of a button for you and what you're used to). You are asking me to abandon my principles ... which I am not willing to do. |
If you merge PRs with merge commits you can just do |
|
I don't want every single commit "introduce trait x" or "make api change to struct y" in master. I can offer that you split out changes you really want to see separately in separate PRs (eg a separate pr for the scriptstate bugfix could make sense), but this is as far as I can go. If you really care this much about the commit style of a project you're not the maintainer of, please fork. To me the ideal of open source means sharing ideas and collaborating on api design (which works as well with two forks), it does not mean having arguments on the internet about tabs vs spaces or whatever this is. I, as the primary author and maintainer of this library, picked a style and that should really the end of the discussion. |
|
Going to close this PR under the assumption that you would not like to see it merged at all if it had to be squash merged. Not sure how to interpret the 👍. Feel free to correct me. Should you change your mind I'm always happy to reevaluate. I have no problems with the code itself or the feature it adds and generally apprechiate your interest and contributions. |
|
Right, sorry for this little dispute. As I stated in my first issue here, I very much appreciate your project. I wouldn't have had the nerve to implement such a huge state machine, so I'm very glad you did it :) And even though I strongly disagree with you when it comes to commit stye, I appreciate that you clearly lay out your stance and are so welcoming to forks. I'll open one last PR for my state transition fix and that will probably conclude my contributions to this repository. |
|
it's all good, and also I just fixed the state transition in master myself 👍 |
Closes #10.