Skip to content

Conversation

kuznetsovmoci
Copy link
Contributor

No description provided.

if (!$skip_failed_test_logs) {
push @{$self->{OUTPUT}}, new Prettify::Failed_Tests_HTML ($basename, $buildname, $rev_link, $log_prefix); #Must be 4, if used
push @{$self->{OUTPUT}}, new Prettify::Failed_Tests_HTML ($basename, $buildname, $self->{FAILED_TESTS}, $rev_link, $log_prefix); #Must be 4, if used
Copy link

@ClaytonCalabrese ClaytonCalabrese Feb 25, 2021

Choose a reason for hiding this comment

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

should this comment be changed?

Copy link
Member

Choose a reason for hiding this comment

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

Or removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is logic that relies on this being #4 - similar to other reports above

Copy link

@ClaytonCalabrese ClaytonCalabrese Feb 26, 2021

Choose a reason for hiding this comment

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

Maybe make the comment more clear. I assumed it meant that it needed 4 variables passed to be used.

Copy link
Contributor Author

@kuznetsovmoci kuznetsovmoci Feb 26, 2021

Choose a reason for hiding this comment

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

I made it more clear, and there is also a context right above, which makes it even more understandable:

    @{$self->{OUTPUT}} =
        (
            new Prettify::Full_HTML ($basename),   #Must be at 0
            new Prettify::Brief_HTML ($basename),
            new Prettify::Totals_HTML ($basename), #Must be at 2
            new Prettify::Config_HTML ($basename), #Must be at 3
        );

Copy link

@ClaytonCalabrese ClaytonCalabrese left a comment

Choose a reason for hiding this comment

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

Only thing I see left to deal with is clarify the comment. Otherwise looks good.

@kuznetsovmoci kuznetsovmoci marked this pull request as ready for review February 26, 2021 19:22
@mitza-oci mitza-oci merged commit ca85976 into DOCGroup:master Feb 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants