Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Store Description value of ConfigurationProperty #41842

Merged
merged 1 commit into from
Oct 17, 2019
Merged

Store Description value of ConfigurationProperty #41842

merged 1 commit into from
Oct 17, 2019

Conversation

Marusyk
Copy link
Member

@Marusyk Marusyk commented Oct 16, 2019

It seems that description parameter isn't used:

public ConfigurationProperty(string name,
Type type,
object defaultValue,
TypeConverter typeConverter,
ConfigurationValidatorBase validator,
ConfigurationPropertyOptions options,
string description)
{
ConstructorInit(name, type, options, validator, typeConverter);
SetDefaultValue(defaultValue);
}

but according to docs it should

Please review and let me know if code authors intentionally don't use it or it is a bug
Thank you in advance

Copy link
Member

@JeremyKuhne JeremyKuhne left a comment

Choose a reason for hiding this comment

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

Nice catch and fix. Thanks!

@JeremyKuhne JeremyKuhne merged commit b9186cf into dotnet:master Oct 17, 2019
@Marusyk Marusyk deleted the marusyk/ConfigurationProperty branch October 17, 2019 07:18
@ViktorHofer
Copy link
Member

@JeremyKuhne you merged the change without any CI running? This broke the build in corefx.

@ViktorHofer
Copy link
Member

@dotnet/dnceng can we find out why CI wasn't triggered for this PR?

@Marusyk Marusyk restored the marusyk/ConfigurationProperty branch October 17, 2019 10:21
@Marusyk
Copy link
Member Author

Marusyk commented Oct 17, 2019

@ViktorHofer Should I create another PR for this?

@mmitche
Copy link
Member

mmitche commented Oct 17, 2019

It appears to be the only PR around that time that didn't trigger. We don't have any insight to what goes on in the AzDO backend. Was it possible there was a merge conflict with the PR that got resolved pror to merge? That would block CI.

@ViktorHofer
Copy link
Member

Maybe @JeremyKuhne can answer Matt's questions.

Should I create another PR for this?

@Marusyk yes please.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants