search

instructure / paseto

A paseto implementation in rust.
MIT License
151 stars 14 forks source link

Use "if let" in builder #17

Closed vbfox closed 4 years ago

vbfox commented 4 years ago

Motivation

Make the code a little more idiomatic in one specific place

Test Plan

Ran cargo test, it still pass.

Mythra commented 4 years ago

Hey thank you for this PR!

I'm happy to merge this since it fixes an inconsistency where token/builders.rs, and token/mod.rs. Where one was using if let, and the other was using: else if { unwrap(); }. However I feel it's important to call out that in general completely idomatic PRs will be rejected. To be more clear if there's two ways of doing the same thing, and no difference between the two I'd prefer to just stick to using one way of doing things (I don't particularly care what the way is).

This would normally be a perfect example of that. There's no real readability difference IMO (both are as readable to someone who's spent a couple minutes with the rust book), one isn't easier to maintain, it doesn't make a difference to a consumer of the api, and there's no noticeable runtime overhead between the two. The reason this is getting merged is because we did it one way in one file, one way in another for two files in the same module.

I'm just outlining this so that way it can save you (or others) some work if someone ever thinks of making a PR similar to this where there isn't an inconsistency they're fixing.

Even still, thank you very much for filing this PR, and your contribution.

vbfox commented 4 years ago

Oh I totally understand that style PRs have a chance of being rejected :)

I'm mostly in the process of learning rust so even what's idiomatic or not isn't very clear for me yet. I'm finding the if let style a little clearer but don't really have enough experience in the language to know what i'll think after really doing it daily for months.