This repository was archived by the owner on Dec 29, 2022. It is now read-only.
Added support for request Id to be either number or string as specifi… #73
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nrc
reviewed
Oct 24, 2016
| fn visit_str<E>(&mut self, value: &str) -> Result<Id, E> where E: de::Error { | ||
| Ok(Id::String(value.to_string())) | ||
| } | ||
| } |
There was a problem hiding this comment.
Nit: can you move all this into deserialize
|
The approach def. looks good. I had one nit that needs fixing and it needs a rebase, but otherwise r+. Can you check that running |
|
Oh, and add a test to the test suite that uses String ids. |
craftytrickster
force-pushed
the
id-variant
branch
from
85dea88 to
7f67b65
Compare
craftytrickster
commented
Oct 25, 2016
| @@ -202,14 +204,20 @@ fn init_env(project_dir: &str) { | |||
| env::set_current_dir(cwd).expect(FAIL_MSG); | |||
| } | |||
|
|
|||
| #[derive(Clone, Debug, PartialEq)] | |||
| enum Id { | |||
There was a problem hiding this comment.
Do you want to reuse the Id struct from the main code, or should there be a separation like here?
There was a problem hiding this comment.
I don't think you need either. I think that each test case should be able to specify an id in ad hoc manner. I.e., they would only test the id if that is what they are testing, if it doesn't matter, then they can use id 0 or 42 or whatever
# for free
to subscribe to this conversation on GitHub.
Already have an account?
#.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This addresses the need to support the id as both a number and a string as identified in - #62.
I am opening the PR to see if this is the approach you would like to take. If you agree, I would like to do more manual testing to make sure this did not break anything prior to merge.