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

Feature/3101 rename config to configuration #3226

Merged

Conversation

HHobeck
Copy link
Contributor

@HHobeck HHobeck commented Oct 12, 2022

  • Rename the class Config to Configuration. Change namespace Configuration to Configurations.
  • Rename the class BranchConfig to BranchConfiguration. Change parameter name config to configuration..

Description

This is a refactoring step without any business logic change.

Related Issue

no related issue.

Motivation and Context

Refactoring of Configuration to have clear naming.

How Has This Been Tested?

All unit and integration tests have been executed.

Screenshots (if appropriate):

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@asbjornu
Copy link
Member

This is great, but please let's discuss changes like this in an issue before you invest time in a PR. I think the namespace should remain GitVersion.Configuration and the Config class can be renamed to GitVersionConfiguration. Thoughts @HHobeck @arturcic?

@arturcic
Copy link
Member

GitVersionConfiguration

Agree with that

@HHobeck
Copy link
Contributor Author

HHobeck commented Oct 12, 2022

This is great, but please let's discuss changes like this in an issue before you invest time in a PR. I think the namespace should remain GitVersion.Configuration and the Config class can be renamed to GitVersionConfiguration. Thoughts @HHobeck @arturcic?

Okay no problem. I can change it back. I did the namespace change because otherwise the namespace conflicts with the class name. Actually I think we are in the domain of GitVersion to name the class GitVersionConfiguration or GitVersionContext seem for me technically motivated. What about OverallConfiguration!? ;)

@asbjornu
Copy link
Member

I think we are in the domain of GitVersion to name the class GitVersionConfiguration or GitVersionContext seem for me technically motivated.

As the configuration file is named GitVersion.yml, I think the name GitVersionConfiguration is quite fitting.

What about OverallConfiguration!? ;)

Not a fan, sorry.

@arturcic
Copy link
Member

@HHobeck I think we need to rename more classes that contain the prefix or suffix Config for example in the folder https://github.com/GitTools/GitVersion/tree/746efd9290c5ca5fc0164ceecc3b4668adda9c68/src/GitVersion.Core/Configuration, or some of the Unit test class names, do you agree?

Copy link
Member

@asbjornu asbjornu left a comment

Choose a reason for hiding this comment

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

Awesome! 👏🏼

@arturcic
Copy link
Member

There are a couple of other classes to be renamed:
OverrideConfigOptionParser
ConfigExtensionsTests
ConfigProviderTests
DefaultConfigFileLocatorTests
IgnoreConfigTests

Copy link
Member

@asbjornu asbjornu left a comment

Choose a reason for hiding this comment

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

Looking good! 👍🏼

@HHobeck
Copy link
Contributor Author

HHobeck commented Oct 14, 2022

Okay I'm done please take a look and give me feedback. I would prefer to hold on and not to refactor anything else in the PR.

@HHobeck
Copy link
Contributor Author

HHobeck commented Oct 14, 2022

Please also notice the implementation of ScratchConfigurationBuilder (I'm not sure if EmptyConfigurationBuilder is maybe better!?) and GitFlowConfigurationBuilder. In my opinion this should be used for unit and integration tests.

src/Directory.Packages.props Outdated Show resolved Hide resolved
@arturcic
Copy link
Member

@asbjornu when you're done with the review please merge this one

@asbjornu asbjornu added this to the 6.x milestone Oct 17, 2022
Copy link
Member

@asbjornu asbjornu left a comment

Choose a reason for hiding this comment

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

Marvellous work @HHobeck! 👏🏼

@asbjornu asbjornu merged commit c43fdb9 into GitTools:main Oct 17, 2022
@mergify
Copy link
Contributor

mergify bot commented Oct 17, 2022

Thank you @HHobeck for your contribution!

@arturcic
Copy link
Member

🎉 This issue has been resolved in version 6.0.0 🎉
The release is available on:

Your GitReleaseManager bot 📦🚀

@HHobeck HHobeck deleted the feature/3101_rename-config-to-configuration branch March 13, 2023 06:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants