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

Convert t/mojo/delay.t to use subtests #1568

Merged

Conversation

dz-at-tc
Copy link
Contributor

@dz-at-tc dz-at-tc commented Oct 5, 2020

Summary

Convert t/mojo/delay.t to use subtests.

Motivation

One more file converted.

References

Issue #1520

jhthorsen
jhthorsen previously approved these changes Oct 6, 2020
Copy link
Member

@jhthorsen jhthorsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like some of the variables are not sorted alphabetically, but they weren't in the original commit either so I'm not going to request changes based on it.

https://github.com/mojolicious/mojo/pull/1568/files?w=1 looks good to me 👍

t/mojo/delay.t Outdated Show resolved Hide resolved
t/mojo/delay.t Outdated Show resolved Hide resolved
t/mojo/delay.t Outdated Show resolved Hide resolved
@dz-at-tc
Copy link
Contributor Author

dz-at-tc commented Oct 6, 2020

A couple of follow up questions:

  1. Should declarations like "my ($failed, $result);" be split into individual my declarations, one per line?
  2. Should I alphabetize the var declarations as @jhthorsen pointed out above?

@dz-at-tc dz-at-tc requested a review from kraih October 8, 2020 13:17
t/mojo/delay.t Outdated

subtest 'Exception in first step' => sub {
my $failed;
my $result;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On separate lines is inconsistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good, thanks for clarifying that the parentheses are preferred!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main preference is consistency. If something is unclear just look at how other files are formatted.

@kraih kraih requested a review from a team October 8, 2020 13:19
@dz-at-tc dz-at-tc force-pushed the add_subtests_to_mojo_delay_test branch from 6b2c3cf to ca13cc7 Compare October 10, 2020 00:16
@kraih kraih requested a review from a team October 10, 2020 17:39
@mergify mergify bot merged commit b7aa7fe into mojolicious:master Oct 10, 2020
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.

None yet

4 participants