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

Boy Scout #5808

Merged
merged 2 commits into from
Feb 22, 2023
Merged

Boy Scout #5808

merged 2 commits into from
Feb 22, 2023

Conversation

BraisGabin
Copy link
Member

These are two minor Boy scouts to remove code that is not longer necessary.

@codecov
Copy link

codecov bot commented Feb 22, 2023

Codecov Report

Merging #5808 (080ff34) into main (1694441) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##               main    #5808   +/-   ##
=========================================
  Coverage     84.61%   84.61%           
  Complexity     3794     3794           
=========================================
  Files           547      547           
  Lines         12922    12922           
  Branches       2264     2264           
=========================================
  Hits          10934    10934           
  Misses          862      862           
  Partials       1126     1126           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@@ -11,4 +11,4 @@ fun yamlConfig(name: String): Config =
resource(name).toURL().openSafeStream().reader().use(YamlConfig::load)

fun yamlConfigFromContent(@Language("yaml") content: String): Config =
StringReader(content.trimIndent()).use(YamlConfig::load)
StringReader(content).use(YamlConfig::load)
Copy link
Member

Choose a reason for hiding this comment

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

Is this a potentially breaking change for users of the test library? Unlikely many affected, and trivial fix, but still, maybe worth a mention in release notes?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would be shocked if someone use this function. But better safe than sorry. I add the notable change tag.

Copy link
Member

Choose a reason for hiding this comment

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

I would be shocked if someone use this function.

Can we remove (i.e. make internal) this function instead of having to mention this in the release notes?

Copy link
Member

Choose a reason for hiding this comment

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

Can we hide it? Isn't it used from other modules?

Copy link
Member

Choose a reason for hiding this comment

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

Yup it's used all over the places so we'll have to keep it. I'm unsure we need to mention this in the 'notable changes' though

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that no one outside this repo uses this function. And, even if they use it, this shouldn't be a breaking change. The yaml should be valid even if you don't remove the identation. So I'm ok if you remove the notable changes tag @cortinico.

@BraisGabin BraisGabin added this to the 1.23.0 milestone Feb 22, 2023
@BraisGabin BraisGabin added the notable changes Marker for notable changes in the changelog label Feb 22, 2023
@BraisGabin BraisGabin merged commit 1e66c6c into main Feb 22, 2023
@BraisGabin BraisGabin deleted the improve-code branch February 22, 2023 14:38
@cortinico cortinico removed the notable changes Marker for notable changes in the changelog label Mar 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants