Skip to content

More information in verbose mode to provide faster feedback loop #390

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Nov 23, 2020
Merged

More information in verbose mode to provide faster feedback loop #390

merged 3 commits into from
Nov 23, 2020

Conversation

@mrnugget
Copy link
Contributor

@mrnugget mrnugget commented Nov 19, 2020 •
edited
Loading

Thoughts/opinions/ideas?

screenshot_2020-11-19_16 23 17

screenshot_2020-11-20_17 59 46

TODOs:

  • Clean up the code

@mrnugget mrnugget requested a review from a team November 19, 2020 15:45
eseliger
eseliger approved these changes Nov 19, 2020
View reviewed changes
Copy link
Member

@eseliger eseliger left a comment

Choose a reason for hiding this comment

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

Since I've been around when this was made, I think I shouldn't approve, but I'm feeling wild so I do it anyways 🦊

LawnGnome
LawnGnome approved these changes Nov 19, 2020
View reviewed changes
Copy link
Contributor

@LawnGnome LawnGnome left a comment

Choose a reason for hiding this comment

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

I think this is great!

I have a few code level suggestions that are probably already encompassed by your "clean up the code" TODO, but I think the feature is awesome.

@mrnugget
Copy link
Contributor Author

I have a few code level suggestions that are probably already encompassed by your "clean up the code" TODO, but I think the feature is awesome.

2/3 I thought of, so I appreciate the third one!

@mrnugget mrnugget force-pushed the mrn/verbose-diff-stat-output branch from 3c76f7b to 81c205c Compare November 20, 2020 16:56
@mrnugget
Copy link
Contributor Author

@LawnGnome @eseliger I cleaned up the code as best as I could today. We still parse the file diffs twice, if a task is completed (once for the verbose log, once for the status bar), but I couldn't make that go away without making the code hard to understand. I'd be happy to give it another shot next week. For now: please review :)

@mrnugget mrnugget changed the title RFC: More information in verbose mode to provide faster feedback loop More information in verbose mode to provide faster feedback loop Nov 20, 2020
LawnGnome
LawnGnome approved these changes Nov 20, 2020
View reviewed changes
Copy link
Contributor

@LawnGnome LawnGnome left a comment

Choose a reason for hiding this comment

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

I cleaned up the code as best as I could today. We still parse the file diffs twice, if a task is completed (once for the verbose log, once for the status bar), but I couldn't make that go away without making the code hard to understand.

That's fair enough — this version's quite a bit cleaner, and I think I'm less concerned about the possible cost of parsing the diffs twice with the verbose checks up front.

Ship it!

internal/campaigns/service.go

ClearCache bool
KeepLogs bool
VerboseLogger bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch!

@mrnugget mrnugget merged commit ac7ec26 into main Nov 23, 2020
@mrnugget mrnugget deleted the mrn/verbose-diff-stat-output branch November 23, 2020 08:29
scjohns pushed a commit that referenced this pull request Apr 24, 2023
@mrnugget
More information in verbose mode to provide faster feedback loop (#390)
c621c56 
* Amazing new verbose mode

* Clean up code

* Add changelog entry
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eseliger eseliger eseliger approved these changes

+1 more reviewer

@LawnGnome LawnGnome LawnGnome approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mrnugget @LawnGnome @eseliger