Skip to content

#850 fix printing all matrix links#866

Merged
piotradamczyk5 merged 6 commits into
masterfrom
#850-print-all-matrix-links
Jul 2, 2020
Merged

#850 fix printing all matrix links#866
piotradamczyk5 merged 6 commits into
masterfrom
#850-print-all-matrix-links

Conversation

@piotradamczyk5
Copy link
Copy Markdown
Contributor

@piotradamczyk5 piotradamczyk5 commented Jun 23, 2020

Fixes #850

Test Plan

How do we know the code works?

All matrices failed links are printed in summary below the summary table.
Do not print any data if there are not present failed links.

Checklist

  • Unit tested
  • release_notes.md updated

@piotradamczyk5 piotradamczyk5 self-assigned this Jun 23, 2020
@piotradamczyk5 piotradamczyk5 marked this pull request as draft June 23, 2020 15:15
@piotradamczyk5 piotradamczyk5 marked this pull request as ready for review June 23, 2020 15:28
@bootstraponline
Copy link
Copy Markdown
Contributor

All matrices links are printed in summary

We want to print only links to failed matrices, not all. The goal is to make it as easy as possible to investigate failures.

@piotradamczyk5
Copy link
Copy Markdown
Contributor Author

All matrices links are printed in summary

We want to print only links to failed matrices, not all. The goal is to make it as easy as possible to investigate failures.

Fixed to print only failures matrices

@piotradamczyk5 piotradamczyk5 marked this pull request as draft June 23, 2020 18:06
Copy link
Copy Markdown
Contributor

@bootstraponline bootstraponline left a comment

Choose a reason for hiding this comment

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

this looks good to me

it would be good to have at least one other member on the team review the PR. 🙂

}
}

private fun Collection<SavedMatrix>.printMatricesLinks(writer: StringWriter) {
Copy link
Copy Markdown
Contributor

@pawelpasterz pawelpasterz Jun 30, 2020

Choose a reason for hiding this comment

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

Here is my proposal for formatting. I am just not a big fan of multiple ?. chain calls. Of course, it's not change request. Just let me know what do you think

Suggested change
private fun Collection<SavedMatrix>.printMatricesLinks(writer: StringWriter) {
private fun Collection<SavedMatrix>.printMatricesLinks(writer: StringWriter) = this
.filter { it.failed() }
.takeIf { it.isNotEmpty() }
?.run {
writer.println("More details are available at:")
forEach { writer.println(it.webLinkWithoutExecutionDetails) }
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I personally preferer to avoid nested operations in block, that's why I changed it to ?. chain (previous version looks like yours :) )

Copy link
Copy Markdown
Contributor

@pawelpasterz pawelpasterz Jul 1, 2020

Choose a reason for hiding this comment

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

Got your point. We can still make it simpler and skip one map operation and just invoke forEach { writer.println(it.webLinkWithoutExecutionDetails) }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

applied your suggestion, because of new line comment :)

@pawelpasterz
Copy link
Copy Markdown
Contributor

Maybe we could add new line or other separator below links? It's cosmetic change but would improve clarity of logs
Screenshot 2020-07-01 at 10 50 19

@piotradamczyk5
Copy link
Copy Markdown
Contributor Author

Maybe we could add new line or other separator below links? It's cosmetic change but would improve clarity of logs
Screenshot 2020-07-01 at 10 50 19

so in that case, you code without ?. will look better :)

Copy link
Copy Markdown
Contributor

@pawelpasterz pawelpasterz left a comment

Choose a reason for hiding this comment

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

Nice work, thanks! 👍

@piotradamczyk5 piotradamczyk5 merged commit 69ff581 into master Jul 2, 2020
@piotradamczyk5 piotradamczyk5 deleted the #850-print-all-matrix-links branch July 2, 2020 07:17
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.

Not all errored matrix links printed

3 participants