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

Rename AttributeDictionary to AttributeValueDictionary #107

Closed
JeremyCaney opened this issue Jun 25, 2022 · 2 comments
Closed

Rename AttributeDictionary to AttributeValueDictionary #107

JeremyCaney opened this issue Jun 25, 2022 · 2 comments
Assignees
Labels
Area: Mapping Relates to one of the `ITopicMappingService` interfaces or implementations. Priority: 1 Severity 2: Major Status 5: Complete Task is considered complete, and ready for deployment. Type: Bug Behavior that is inconsistent with documented or expected behavior.
Milestone

Comments

@JeremyCaney
Copy link
Member

There is already a class named AttributeDictionary in Microsoft.AspNetCore.Mvc.ViewFeatures namespace (reference). Unfortunately, the ViewFeatures namespace is often appropriate to introduce as a global namespace for view models. That introduces a naming conflict in the exact domain we expect to be using our AttributeDictionary.

The AttributeDictionary naming convention makes perfect sense for our needs. But to disambiguate it, we may want to rename it to AttributeValueDictionary. This isn't ideal, as it's inconsistent with most other attribute-related naming conventions in OnTopic as well as dictionary names in .NET. That said, there is at least some precedence for this in the ASP.NET Core MVC RouteValueDictionary, which is a mapping of route parameter names to route parameter values. That's not especially convincing, but as a popular class it at least provides some familiarity or justification for this convention, instead of it being entirely arbitrary.

Originally posted by @JeremyCaney in #99 (comment)

@JeremyCaney JeremyCaney self-assigned this Jun 25, 2022
@JeremyCaney JeremyCaney added Type: Bug Behavior that is inconsistent with documented or expected behavior. Area: Mapping Relates to one of the `ITopicMappingService` interfaces or implementations. Severity 2: Major Priority: 1 Status 2: Scheduled Planned for an upcoming release. labels Jun 25, 2022
@JeremyCaney JeremyCaney added this to the OnTopic 5.2.0 milestone Jun 25, 2022
@JeremyCaney
Copy link
Member Author

This was resolved in 0b62fe0.

@JeremyCaney JeremyCaney added Status 5: Complete Task is considered complete, and ready for deployment. and removed Status 2: Scheduled Planned for an upcoming release. labels Jun 25, 2022
JeremyCaney added a commit to OnTopicCMS/OnTopic-Editor-AspNetCore that referenced this issue Jun 25, 2022
The `AttributeDictionary` class (OnTopicCMS/OnTopic-Library#99) introduces a conflict with the `AttributeDictionary` in `Microsoft.AspNetCore.Mvc.ViewFeatures`, which is often used as a namespace in model libraries (OnTopicCMS/OnTopic-Library#107). To disambiguate this, it's been renamed to `AttributeValueDictionary`. The editor's view models have been updated to utilize this new naming convention.
JeremyCaney added a commit to OnTopicCMS/OnTopic-Editor-AspNetCore that referenced this issue Jun 25, 2022
The `AttributeDictionary` class (OnTopicCMS/OnTopic-Library#99) introduces a conflict with the `AttributeDictionary` in `Microsoft.AspNetCore.Mvc.ViewFeatures`, which is often used as a namespace in model libraries (OnTopicCMS/OnTopic-Library#107). To disambiguate this, it's been renamed to `AttributeValueDictionary` in OnTopic Library 5.2.0 Alpha 255 (b243853). The editor's view models have been updated to utilize this new naming convention (1ca4eb4).
JeremyCaney added a commit that referenced this issue Jun 25, 2022
Previously, `OnTopic.Attributes` couldn't be (easily) introduced as a global using directive (c9f6b17, #93) because of a naming conflict with `AttributeDictionary`, which existed in both `OnTopic.Attributes` and `Microsoft.AspNetCore.Mvc.ViewFeatures`. With `AttributeDictionary` renamed to `AttributeValueDictionary` (0b62fe0, #107), this can be reintroduced.
JeremyCaney added a commit that referenced this issue Jun 25, 2022
With `AttributeDictionary` renamed to `AttributeValueDictionary` (#107) the potential naming conflict with the out-of-the-box `AttributeValueDictionary` is removed, and we can thus (re)introduce `OnTopic.Attributes` as a global using directive, and remove the local using directives across `OnTopic` and `OnTopic.Tests` (07d46ec).
JeremyCaney added a commit that referenced this issue Jun 25, 2022
Renamed `AttributeValueDictionary` back to `AttributeDictionary`, thus reverting 38c54c3. It seems this decision was premature, and we can mitigate the naming conflicts without renaming the class, since the out-of-the-box `ViewFeatures` namespace is almost exclusively used in views, and the `AttributeDictionary` is almost exclusively used in view models. This effectively undoes #107.
JeremyCaney added a commit to OnTopicCMS/OnTopic-Editor-AspNetCore that referenced this issue Jun 26, 2022
Previously, `OnTopic.Attributes` couldn't be (easily) introduced as a global using directive (e37de82, 1e83841; #53, OnTopicCMS/OnTopic-Library#93) because of a naming conflict with `AttributeDictionary`, which existed in both `OnTopic.Attributes` and `Microsoft.AspNetCore.Mvc.ViewFeatures`. With `AttributeDictionary` renamed to `AttributeValueDictionary` (OnTopicCMS/OnTopic-Library#107), this can be reintroduced.
JeremyCaney added a commit to OnTopicCMS/OnTopic-Editor-AspNetCore that referenced this issue Jun 26, 2022
With `AttributeDictionary` renamed to `AttributeValueDictionary` (OnTopicCMS/OnTopic-Library#107) the potential naming conflict with the out-of-the-box `AttributeValueDictionary` is removed, and we can thus (re)introduce `OnTopic.Attributes` as a global using directive (0ca571a), and remove the local using directives across `OnTopic.Editor.AspNetCore.Attributes` (f87ad72).
JeremyCaney added a commit to OnTopicCMS/OnTopic-Editor-AspNetCore that referenced this issue Jun 26, 2022
Updated OnTopic Library to the #260 build of the `revert-AttributeValueDictionary` branch. This reverts `AttributeValueDictionary` back to `AttributeDictionary`, in preparation to undo OnTopicCMS/OnTopic-Library#107). The conflict that was trying to avoid wasn't necessary.
JeremyCaney added a commit to OnTopicCMS/OnTopic-Editor-AspNetCore that referenced this issue Jun 26, 2022
The latest patch renamed `AttributeValueDictionary` back to `AttributeDictionary`, thus reverting a large part of the previous patch (b243853). It seems this decision was premature, though, as we can mitigate the naming conflicts without renaming the class, since the out-of-the-box `ViewFeatures` namespace is almost exclusively used in views, and the `AttributeDictionary` is almost exclusively used in view models. This effectively undoes 1ca4eb4 and OnTopicCMS/OnTopic-Library#107.
@JeremyCaney
Copy link
Member Author

JeremyCaney commented Jun 26, 2022

This appears to be based on either an outdated issue or a faulty initial analysis.

If I recall, the initial issue was relevant to cshtml files, which rely heavily on classes in the ViewFeatures namespace (as the name suggests), and thus rely on that namespace being in scope. That said, the OnTopic.Attributes namespace is rarely needed in cshtml files since it pertains to the entity. And while AttributeDictionary is used by view models, it is exclusively used as a piece of scaffolding for the ITopicMappingService, and isn't publicly exposed via the view models, or accessed via the views. As a result, the scenarios where the two AttributeDictionary classes are used in the same context should be incredibly rare. (It does happen, but mostly when intercepting the view logic, as is done via TopicViewResult and TopicViewResultExecutor—which is a backend implementation detail, and not a common scenario for implementations).

It is conceivable that something has changed in how the .NET SDK handles global using directives with ASP.NET Core applications, thus resolving conflicts that previously existed (e.g., by exposing global using directives to cshtml files). That said, this isn't confirmed.

Regardless, this conflict doesn't appear to be an issue, and thus we should revert this work, preferring AttributeDictionary to AttributeValueDictionary. This has been done on the OnTopic Library (c0152cc) and the OnTopic Editor (be0b76c).

JeremyCaney added a commit that referenced this issue Jun 26, 2022
Renamed `AttributeValueDictionary` back to `AttributeDictionary`, thus reverting 38c54c3. This decision was made prematurely, and we can mitigate the naming conflicts without renaming the class. This is because the out-of-the-box `ViewFeatures` namespace is almost exclusively used in views, and the `AttributeDictionary` is almost exclusively used in view models. This effectively undoes #107, while still allowing `OnTopic.Attributes` to be added as a global using directive, where appropriate.
JeremyCaney added a commit to OnTopicCMS/OnTopic-Editor-AspNetCore that referenced this issue Jun 26, 2022
Renamed `AttributeValueDictionary` back to `AttributeDictionary`, thus reverting cec9de4. This decision was made prematurely, and we can mitigate the naming conflicts without renaming the class. This is because the out-of-the-box `ViewFeatures` namespace is almost exclusively used in views, and the `AttributeDictionary` is almost exclusively used in view models. This effectively undoes OnTopicCMS/OnTopic-Library#107, while still allowing `OnTopic.Attributes` to be added as a global using directive, where appropriate.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Mapping Relates to one of the `ITopicMappingService` interfaces or implementations. Priority: 1 Severity 2: Major Status 5: Complete Task is considered complete, and ready for deployment. Type: Bug Behavior that is inconsistent with documented or expected behavior.
Projects
None yet
Development

No branches or pull requests

1 participant