Skip to content

Make library more rusty #17

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

Open
wants to merge 31 commits into
base: v0.6
Choose a base branch
from
Open

Conversation

hellow554
Copy link

This PR is a huge beast and I'm somewhat sorry for that.

What I did is basicly running cargo fmt and afterwards cargo clippy.
This will improve, at least for me, the code quality a lot.

First, for people like me, who are reading unfamiliar code a lot, having a common code style is amazing. I can easily skip over the code without thinking too much, because my brain knows how a function looks like, if and else braces are always on the same line and so on.
I know, not everybody like that, but it's what the rust community has decided on.

Second, using clippy hugely improves the code quality. Not just style, but sometimes also speed and compiler optimizations.

I tried to give every lint a seperate commit, so you can view it commit by commit and mark everything you may like or don't like.
Not every lint must be accepted in every situation but then it should be marked with [allow(clippy::xxx)] and a comment why.

I'm happy to hear from you.

@hellow554
Copy link
Author

Hey @Logicalshift

have you had any time looking into this?

@Logicalshift
Copy link
Owner

Yes, but it's tricky and I've avoided commenting because I have a really strong reaction to what rustfmt does: I really hate it, but that's no good as the basis of a discussion so I've been trying to put my thoughts in a bit more order, because I also can't disagree that there's a few (or even a lot) of places where the layout can be improved.

I find the changes that clippy makes much less objectionable, though I think I might turn a few fixes off.

On rustfmt: it's taking me quite a while to figure this out. Part of it is that the flo part of flo_curves comes from FlowBetween so you're asking me to make a much bigger change to a much wider codebase here.

Part of it is I work commercially and that often involves implementing things in a way my team is more comfortable with rather than the way I think is best, which is often quite frustrating, so a part of what motivates me to work on my own stuff on my own time is the freedom to go off the beaten path.

Part of it is that flo_curves is four years old and I'm pretty familiar with the codebase now: quite a lot of the formatting changes are there because of things I wanted to see or emphasise when debugging some gnarly issue with solving cubic equations, which is always a bit hellish - every one boils down to 'maths error', 'floating point precision issue' or 'logic bug' and require a lot of context switching, and when I'm switching between code and a maths paper I really want the relationship to stand out.

I think the main thing, though is that FlowBetween has an aspirational goal: it's trying to do things better than they've been done before, and that influences the project at all levels, including code formatting. I've no real objection to what rustfmt does, it's just it would go against the goals of the project if I didn't try to do better. It might not always succeed, but there are definite benefits when it does (and even the cases where things don't work can yield interesting insights)

So that's why I think the first thing I'm going to do is get the clippy changes in, and why it's taking me a while to decide what to do next. Your criticism over the formatting isn't invalid: there are definitely places where things don't take rustfmt as a baseline that probably need to be fixed up, though I think my intention is more to do that with an eye to improving formatting and style in general - which is going to be a longer process than just running the tool but should yield higher-quality improvements.

I think that is probably necessary even if the end result is just whatever rustfmt does because I do notice some places where the change in formatting does obscure some structure or meaning here, or where it creates formatting that is really ugly because of how a statement is structured.

I've also started putting together a STYLE.md file that will eventually make it to the other parts of FlowBetween to try to explain a bit better what's going on.

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