[TO CLOSE] Add pagination parameter for GET sentences #61
Conversation
jean553
changed the title
|
This PR is now available for review. |
| #[derive(FromForm)] | ||
| struct LastId { | ||
| pub id: UUID, | ||
| } |
There was a problem hiding this comment.
we need this because we can't implement a trait (FromForm) on a type ( UUID) that don't belong to us ?
There was a problem hiding this comment.
if so, it should be an issue raised to the rocket_contrib/uuid crate , as this crate is specifically made to ease interaction with uuid in rocket, so it should do that for us.
There was a problem hiding this comment.
I'm gonna do a second check on this point.
| ORDER BY | ||
| added_at, | ||
| sentence.id | ||
| LIMIT 100 | ||
| LIMIT 10 |
| @@ -128,19 +139,19 @@ fn get_all_sentences<'r>( | |||
| structure::text | |||
| FROM sentence | |||
| JOIN language ON (sentence.language_id = language.id) | |||
| WHERE sentence.id > $1 | |||
There was a problem hiding this comment.
your ids are not sequential , so if a new id get inserted you will certainly miss it.
| let mut response = reqwest::get(&url).unwrap(); | ||
| let url = Url::parse_with_params( | ||
| &url, | ||
| &[("id", "00000000-0000-0000-0000-000000000000")], |
There was a problem hiding this comment.
it's not a valid uuid per-rfc so it should not work actually...
|
|
||
| /* insert multiple sentences with different | ||
| content and "consecutives" uuids, | ||
| from a full-zeros uuid to x-x-x-x-...13 */ |
There was a problem hiding this comment.
something is wrong if you need to make consecutive uuid (see my comment above)
|
|
||
| /* insert multiple sentences with different | ||
| content and "consecutives" uuids, | ||
| from a full-zeros uuid to x-x-x-x-...13 */ |
There was a problem hiding this comment.
something is wrong if you need to make consecutive uuid (see my comment above) , i.e you're tricking the reality to please your test though you should do the oppositve (i.e in reality multiple sentences add in order A B C don't have any garantee to have uuid in order A B C )
| const SENTENCE_MAX_INDEX: usize = 15; | ||
| for id in 1..SENTENCE_MAX_INDEX { | ||
|
|
||
| /* ensure the uuids strings are all valid uuids */ |
There was a problem hiding this comment.
why is the code below ensuring that ?
| assert_eq!( | ||
| sentences.len(), | ||
| 3, | ||
| ); |
There was a problem hiding this comment.
80 lines for a test, with a lot of mundane (i.e low non-functional details ) happening , makes the test very obscure to understand.
There was a problem hiding this comment.
don't hesitate to create helper functions to outsource low details outside of the "reading" path.
There was a problem hiding this comment.
Right, will be done in a separated dedicated PR #62.
jean553
changed the title
|
This PR will be replaced by consecutive small PRs. |
I still have to implement interface tests.