Skip to content
This repository has been archived by the owner on Apr 30, 2024. It is now read-only.

Fix tokens module when not all features are enabled #23

Closed
wants to merge 1 commit into from
Closed

Fix tokens module when not all features are enabled #23

wants to merge 1 commit into from

Conversation

vbfox
Copy link
Contributor

@vbfox vbfox commented Feb 23, 2020

Motivation

The crate currently doesn't build when the easy_tokens feature is present with v1 or v2 only.

Test Plan

Verified it build in all the interesting combinations:

cargo build --no-default-features --features v1,v2
cargo build --no-default-features --features v1,easy_tokens
cargo build --no-default-features --features v2,easy_tokens

Next Steps

I wonder if the current code duplication (And associated problems/bugs) are worth the price. Especially the possibility of excluding the currently default v2 version.

@vbfox vbfox changed the title Fix tokens module when not all features are enabled Fix tokens module when not all features are enabled Feb 23, 2020
@vbfox vbfox requested a review from Mythra February 23, 2020 16:23
@vbfox vbfox mentioned this pull request Feb 23, 2020
@Mythra
Copy link
Collaborator

Mythra commented Feb 23, 2020

Hey thanks for this, I appreciate the PR. Could you rebase so it can merged? Thanks.

Also I'm unclear why:

I wonder if the current code duplication (And associated problems/bugs) are worth the price. Especially the possibility of excluding the currently default v2 version.

Was said? The "code duplication" is incredibly minimal, and really is just the function signature that's being enabled for each specific feature. Where is this price you see it coming from? If it's with long term growth. PASETO only has two supported versions at a time. So we won't just be adding functions forever. In fact the function could will stay the same. Also is there an easy way you can envision to change this? What would you like to see that keeps this feature?

I think it's also critically important to be able to have this feature. For example if you need to meet FIPS requirements. You need to have your "source" builds blessed. The easiest way to do this with paseto is to use V1. Which is backed by OpenSSL (and provides FIPS complaint builds). In this case I would want to turn off support for V2 to ensure no one can or has used them. However, I still might want to use the builder interface.

That being said yes, we absolutely need to test these features if we're going to support them. We shouldn't say we support something if we don't test it. Also I need to get continuous integration back up. So I'm not checking out these things locally, heh. Do you mind filing me an issue that is just: "test the crate in all configurations", that references this PR? Thanks.

@vbfox
Copy link
Contributor Author

vbfox commented Feb 23, 2020

Yes the code duplication was technically minimal but each addition would make it more problematic (either of an option in the library or of a new version of the specification)

I'm quite the newbie in rust so I didn't see the solution while writting this PR but I actually found a nicer way to do that, removing all duplication. So i'll close this one, and suggest PR #24 instead 😁

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants