Add OutputElementHintAttribute to control design time IntelliSense for TagHelpers. by NTaylorMullen · Pull Request #430 · aspnet/Razor

NTaylorMullen · 2015-06-23T18:56:43Z

Decided to not have the attribute inheritable since TargetElement is not inheritable.
Added tests to validate serialization, deserialization and construction of TagHelperDescriptors with OutputElementHints.
Add OutputElementHintAttribute to control inherited TagHelper IntelliSense. #382

NTaylorMullen · 2015-06-23T18:56:53Z

Decided to not have the attribute inheritable since TargetElement is not inheritable.

NTaylorMullen · 2015-06-23T18:57:01Z

dougbu · 2015-06-25T15:35:07Z

Doubt the GetCustomAttribute<T>() is so expensive that it needs to be inside this condition. Also this factory should only execute once at runtime. So calculate the hint unconditionally.

Why waste cycles? The intention of this attribute is that it's design-time only.

I made this suggestion because it seemed like an unnecessary optimization that mixed the hint with descriptors and required another non-var variable.

I'd be more tempted to have a whole property called DesignTimeStuff that then had properties for each of the design-time only parts of the descriptor, which for now might be just the hint.

Like it. UsageDescriptor would be in there too.

Could revamp TagHelperUsageDescriptor to be the design time stuff property. Would just have to add an extra type maybe like TagHelperElementUsageDescriptor which would contain OutputElementHint for TagHelperDescriptors. TagHelperAttributeDescriptors would continue to use TagHelperUsageDescriptor.

What do you guys think?

I don't know what these things are, so I don't think much 😄

dougbu · 2015-06-25T16:09:32Z

⌚

NTaylorMullen · 2015-06-29T23:05:46Z

Updated.

- Added platform specific path handling to the TagHelperUsageDescriptorFactoryTests.

Eilon · 2015-06-30T16:26:00Z

Should this avoid the ctor and just have get/set properties? I'm sure we'll add lots more stuff in the future, and it might be annoying to have to keep making the ctor have more and more params.

In fact, same goes for the other descriptors.

It's deffffinitely annoying. The only reason why this descriptor uses the ctor to initialize everything is because the TagHelperDescriptor and TagHelperAttributeDescriptor do; aka for consistency.

At the initial creation of the descriptors I had them all constructorless but after a while the constructors were added and have stuck around. @dougbu If I remember correctly you showed the interest in adding the constructors initially which made sense at the time. Now that there's a boat load of properties on all of our descriptors how do you feel about nuking the ctors?

Will file an issue if @dougbu agrees (changing from ctors to no ctors will be a lot of noise + require changes to all of our comparers).

Not only are there a boat load of properties, they're not all required.

I'm fine treating that as a separate issue.

We moved toward constructor parameters to make the descriptors immutable while allowing serialization / deserialization. Comparisons, hash codes, and so on get messy if the descriptors can change. I know we wouldn't change the descriptors but we do store some of them in HashSets and so on.

So fine to open an issue but I'm not sure we should ever fix it 😈

The containing descriptors are compared and / or stuck into HashSets.

If others feel strongly about changing TagHelperAttributeDesignTimeDescriptor and TagHelperDesignTimeDescriptor to be mutable, suggest adding the public setters in this PR. Leave #441 to cover the existing TagHelperAttributeDescriptor and TagHelperDescriptor. This is possible / easy because the usage descriptors are only compared in test code.

I'm presuming that the descriptors are never, ever mutated, so in practice this isn't an issue.

This PR is 7 days old. Let's get it in and change the classes later.

Eilon · 2015-06-30T16:27:35Z

Overall looks good, just had a question about the class design.

dougbu · 2015-06-30T17:59:11Z

Keep the ?. operator together

dougbu · 2015-06-30T18:27:45Z

We've got a buried discussion starting here

dougbu · 2015-06-30T18:27:55Z

⌚

NTaylorMullen · 2015-06-30T19:01:57Z

Updated ?. spacing and comment.

dougbu · 2015-06-30T19:07:39Z

(I wasn't waiting on those minor comments 😈)

…r TagHelpers. - Decided to not have the attribute inheritable since TargetElement is not inheritable. - Added tests to validate serialization, deserialization and construction of TagHelperDescriptors with OutputElementHints. - Changed TagHelperUsageDescriptor to TagHelperDesignTimeDescriptor and TagHelperAttributeDesignTImeDescriptor. #382

dnfclas added the cla-already-signed label Jun 23, 2015

NTaylorMullen reviewed Jun 23, 2015
View reviewed changes

dougbu reviewed Jun 25, 2015
View reviewed changes

NTaylorMullen force-pushed the nimullen/consumedocs.352 branch from f6b56d1 to 9055da1 Compare June 25, 2015 22:46

Quick fix: Do not check a platform-specific message

308eddc

NTaylorMullen force-pushed the nimullen/elementhint.382 branch 2 times, most recently from 5845c45 to 54481ac Compare June 29, 2015 23:05

Fix Travis-CI.

eda9edc

- Added platform specific path handling to the TagHelperUsageDescriptorFactoryTests.

Eilon reviewed Jun 30, 2015
View reviewed changes

NTaylorMullen mentioned this pull request Jun 30, 2015

Change all TagHelper related descriptors to not use constructors. #441

Closed

dougbu reviewed Jun 30, 2015
View reviewed changes

NTaylorMullen force-pushed the nimullen/elementhint.382 branch from 54481ac to 4afb7b6 Compare June 30, 2015 19:01

NTaylorMullen force-pushed the nimullen/elementhint.382 branch from 4afb7b6 to 85be731 Compare June 30, 2015 19:13

NTaylorMullen closed this Jun 30, 2015

NTaylorMullen deleted the nimullen/elementhint.382 branch June 30, 2015 19:50

Conversation

NTaylorMullen commented Jun 23, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NTaylorMullen commented Jun 23, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougbu commented Jun 25, 2015

Uh oh!

NTaylorMullen commented Jun 29, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Eilon commented Jun 30, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougbu commented Jun 30, 2015

Uh oh!

dougbu commented Jun 30, 2015

Uh oh!

NTaylorMullen commented Jun 30, 2015

Uh oh!

dougbu commented Jun 30, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants