Skip to content

Conversation

@joshuacookdev
Copy link
Contributor

Addresses #3282.

Nothing too major, just a simple enum renaming as requested. I'd like to address this question before merging, if possible, but if you guys just want to merge and move on, that's fine with me.

Copy link
Member

@augustoproiete augustoproiete left a comment

Choose a reason for hiding this comment

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

@joshuacookdev Thanks for PR! One small change needed (see comment) and then it's good to go.

Copy link
Member

@augustoproiete augustoproiete left a comment

Choose a reason for hiding this comment

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

LGTM. Could you squash the commits and push --force?

Copy link
Member

@augustoproiete augustoproiete left a comment

Choose a reason for hiding this comment

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

LGTM! Will be merged for Cake v2.0

@augustoproiete
Copy link
Member

Hey @joshuacookdev, we're preparing to release Cake 2.0 and we can merge this PR now. Could you please rebase on top of current develop?

@augustoproiete
Copy link
Member

I reached out to @joshuacookdev via DM on Twitter to help with rebasing his PR (as we don't have permission to do ourselves). If we don't hear from him in the next couple of days I'll open a new PR with the same commit so we can merge

@joshuacookdev
Copy link
Contributor Author

Hey @augustoproiete - I've been out of town this weekend but was planning to rebase and push tonight. Sorry for the hold-up!

@joshuacookdev joshuacookdev marked this pull request as ready for review October 12, 2021 03:26
@joshuacookdev
Copy link
Contributor Author

joshuacookdev commented Oct 12, 2021

@augustoproiete rebased and pushed, followed through to make sure tests passed, and there seems to have been an issue on the Azure Build Cake Mac check. I don't think I have the capability to rerun the build, so figured I'd bring it to your attention

https://dev.azure.com/cake-build/Cake/_build/results?buildId=21212&view=results

@augustoproiete
Copy link
Member

@joshuacookdev Thanks for the heads-up. I re-ran the job that failed and we're green 🟢 again

@gep13
Copy link
Member

gep13 commented Oct 12, 2021

@augustoproiete @joshuacookdev just having a quick look at this.... do we need to update the descriptions for the enumerated values as well? I don't think they align properly anymore.

@augustoproiete
Copy link
Member

@gep13 It doesn't seem that we can easily tell as the GitVersion docs doesn't give any details...

image

image

@arturcic Would you be able to confirm that the description we have on the enums are still correct?

@arturcic
Copy link
Contributor

@augustoproiete I will check and will let you know

@devlead
Copy link
Member

devlead commented Oct 12, 2021

Looking at GitVersion the logic essentially is

if (verbosity > Verbosity)
{
    return;
}

and the enum is

public enum Verbosity
{
    Quiet = 0,
    Minimal = 1,
    Normal = 2,
    Verbose = 3,
    Diagnostic = 4
}

which should mean

  • Quiet (Quiet)
  • Minimal (Quiet, Minimal)
  • Normal (Quiet, Minimal, Normal)
  • Verbose (Quiet, Minimal, Normal, Verbose)
  • Diagnostic (Quiet, Minimal, Normal, Verbose, Diagnostic)

@arturcic
Copy link
Contributor

I would suggest to use the same descriptions as the Cake Verbosity uses, as they have the same meaning

@augustoproiete
Copy link
Member

I would suggest to use the same descriptions as the Cake Verbosity uses, as they have the same meaning

👀

https://github.com/cake-build/cake/blob/v1.3.0/src/Cake.Core/Diagnostics/Verbosity.cs

image

@joshuacookdev
Copy link
Contributor Author

joshuacookdev commented Oct 12, 2021

To add a point to the discussion, GitVersion has their own documentation formatted that plainly, also.

https://github.com/GitTools/GitVersion/blob/d94f35cbdc4de396347c8839f8d064523eb0e388/src/GitVersion.Core/Logging/Verbosity.cs

image


If that's preferred, I can very easily update. Just let me know and I'll include it in the squash/rebase when I update the PR again.

Copy link
Member

@pascalberger pascalberger left a comment

Choose a reason for hiding this comment

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

As decided on todays meeting comments for GitVersionVerbosity should match the description GitVersion uses for help output on the console.

@augustoproiete augustoproiete self-requested a review October 17, 2021 19:33
@augustoproiete
Copy link
Member

augustoproiete commented Oct 17, 2021

@joshuacookdev Could you please update the GitVersionVerbosity enum to match the following, and rebase on top of develop?

// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

namespace Cake.Common.Tools.GitVersion
{
    /// <summary>
    /// The GitVersion verbosity. Default is <see cref="Normal"/>.
    /// </summary>
    public enum GitVersionVerbosity
    {
        /// <summary>
        /// Quiet verbosity.
        /// </summary>
        Quiet = 0,

        /// <summary>
        /// Minimal verbosity.
        /// </summary>
        Minimal = 1,

        /// <summary>
        /// Normal verbosity (Default).
        /// </summary>
        Normal = 2,

        /// <summary>
        /// Verbose verbosity.
        /// </summary>
        Verbose = 3,

        /// <summary>
        /// Diagnostic verbosity.
        /// </summary>
        Diagnostic = 4,
    }
}

Copy link
Member

@augustoproiete augustoproiete left a comment

Choose a reason for hiding this comment

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

@augustoproiete augustoproiete marked this pull request as draft October 18, 2021 21:09
@joshuacookdev joshuacookdev marked this pull request as ready for review October 19, 2021 16:22
Copy link
Member

@pascalberger pascalberger left a comment

Choose a reason for hiding this comment

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

LGTM

@pascalberger pascalberger changed the title Renamed GitVersionVerbosity values to match GitVersion (GH-3282) Renamed GitVersionVerbosity values to match GitVersion Oct 19, 2021
Copy link
Member

@augustoproiete augustoproiete left a comment

Choose a reason for hiding this comment

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

LGTM

@augustoproiete augustoproiete merged commit f55c45f into cake-build:develop Oct 19, 2021
@augustoproiete
Copy link
Member

@joshuacookdev your changes have been merged, thanks for your contribution 👍

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.

GitVersion Tool: Rename verbosity values to match GitVersion values

6 participants