Skip to content
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

Lwt sequence tests 2 #541

Merged
merged 4 commits into from
Jan 16, 2018
Merged

Lwt sequence tests 2 #541

merged 4 commits into from
Jan 16, 2018

Conversation

cedlemo
Copy link
Contributor

@cedlemo cedlemo commented Jan 15, 2018

#406

@aantron ,

I have a coverage of 99.3%, it just remains the Lwt_sequence.length last branch that is not visited. I can make a PR to delete this branch in it if you want.

Don't merge this one now, I want to change the filled_sequence content with something better than -3,-2,-1,1,2,3 -> 1,2,3,4,5,6. Do you have some requests in order to improve the tests ?

I saw that you use a lot of begin ... end while I use (...), which one is better ? (readability or other reason)

@aantron
Copy link
Collaborator

aantron commented Jan 15, 2018

Do you have some requests in order to improve the tests ?

No, I think the tests are pretty much fine :)

it just remains the Lwt_sequence.length last branch that is not visited. I can make a PR to delete this branch in it if you want.

Please :)

I saw that you use a lot of begin ... end while I use (...), which one is better ? (readability or other reason)

I usually use (...) for single-line expressions, and begin ... end for complex, multi-line expressions.

The reason is because for a complex expression, if using only (...), it is likely to end up with ))))) at the end, which is both difficult to read and annoying in diffs. The next alternative is

blahblah (
  ...
) (* <------ argh *)

but as you can see by my comment, I think this is really ugly :p I think if you have to put a parenthesis on its own line to avoid ))))), it is time to switch to begin ... end.

@cedlemo cedlemo force-pushed the lwt_sequence_tests_2 branch from 722da34 to 548277c Compare January 16, 2018 22:21
@cedlemo
Copy link
Contributor Author

cedlemo commented Jan 16, 2018

@aantron, rebase done and ready to be merged.

@aantron aantron merged commit 77c6522 into ocsigen:master Jan 16, 2018
@aantron
Copy link
Collaborator

aantron commented Jan 16, 2018

Thank you!

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