cessen / ropey

A utf8 text rope for manipulating and editing large texts.
MIT License
1.04k stars 46 forks source link

Update to 2018 edition, and fix all clippy lints #32

Closed aDotInTheVoid closed 4 years ago

aDotInTheVoid commented 4 years ago

Aside from moving to the latest edition, there are a number of other Clippy warnings I've addressed

cessen commented 4 years ago

First: thanks for putting all this effort into these changes!

However, I'm specifically not looking to move Ropey to the 2018 edition. Don't get me wrong, I've moved other projects. But in the case of Ropey it doesn't seem necessary or useful. At least not yet.

I'll comment in-line on the other changes. Some of them I would definitely like to include.

cessen commented 4 years ago

I've gone through and reviewed everything. For each type of change, I only commented on it once, but it applies to every change like it.

Thanks again for your work on this!

aDotInTheVoid commented 4 years ago

I've addressed the changes requested. The note about the druid branch is as I'm going to be maintaining a fork that derives druid::Data, which can't be upstreamed for stability reasons

cessen commented 4 years ago

@aDotInTheVoid Just that one last comment to address, and then it's good to go! Thanks for putting the work into this. I really appreciate it, and it's a nice cleanup of the code.

In the future, if you could keep references to other projects out of commit messages I would appreciate it. It's not a big deal, but it does potentially make things confusing for other people down the line when they're reading the commit logs without any knowledge of your project.

Lastly, I'm pretty sure you don't need to maintain a fork of Ropey for your project. It's not my project, of course, so I don't mean to dictate anything. But if it were me, I would just wrap Rope in a new type and implement the traits I need on top of that.

In general (there might be one or two exceptions) I don't plan to implement traits from other crates in Ropey, even if they're stable. There are just too many crates that people might want traits implemented from. Imagine having traits from every GUI crate, current and future, implemented directly in Ropey. It would get messy and unwieldy pretty quickly.

So my expectation is that people using Ropey (myself included, in my own projects) will take the new-type-wrapper approach. You're welcome to maintain a fork instead if that's what you prefer, of course. Again, I'm not trying to dictate anything. Just trying to provide a helpful recommendation.

aDotInTheVoid commented 4 years ago

Fixed.

I think a fork is necessary because I need to derive the Data, which would rely on [Arc::ptr_eq]. But as a consumer of the library (in the newtype case), I have no knowledge of the internal Arc.

In theory, ropey a fork could be avoided if upstream ropey exposed the Arc::ptr_eq, but this would lock you into needing to keep using Arc for internals for all of 1.0 for semver. Also I don't want to know if you want to expose that sort of API. And anyway, the fork wouldn't diverge too much from upstream, so I'll be fine.

As to you're point about commit messages, I probably should. However, also not to dictate, but I'd use a squash merge, as there are way too many commits for such a small patch

cessen commented 4 years ago

Thanks so much!

That's a good point, this PR probably should be squashed. Thanks for the suggestion!

I think a fork is necessary because I need to derive the Data, which would rely on [Arc::ptr_eq].

Ah, interesting. I had looked at the Data trait earlier, but missed that its equality comparison was supposed to be cheap.

I don't think Ropey actually needs to expose the underlying pointer type to allow for this use-case. I think, instead, it can just provide a method for checking if two Ropes are actually the same data instance (and would use a pointer comparison internally). You won't be able to auto-derive the Data impl that way, but you can manually implement it pretty easily, then.

I also suspect that such a method would be useful in other corner-cases as well. I'll work on adding that the next chance I get.