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

"ContentDefinition.PopulateSettings" inconsistent documentation #15397

Closed
sarahelsaig opened this issue Feb 24, 2024 · 2 comments · Fixed by #15618
Closed

"ContentDefinition.PopulateSettings" inconsistent documentation #15397

sarahelsaig opened this issue Feb 24, 2024 · 2 comments · Fixed by #15618
Labels
Milestone

Comments

@sarahelsaig
Copy link
Contributor

Describe the bug

The documentation comment of ContentDefinition.Settings says

Do not access this property directly. Migrate to use GetSettings and PopulateSettings.

But PopulateSettings has been removed in #14572. This makes the property documentation confusing, and also makes migration to 1.9.0 harder. I see that in the PR usages of the method have been expanded into direct access of ContentDefinition.Settings.

Discussion

Based on the above linked change in #14572, is the "Do not access this property directly." instruction no longer applicable?
Also, is it safe to access the settings like partFieldDefinition.Settings.ToObject<BooleanFieldSettings>() instead of of partFieldDefinition.GetSettings<BooleanFieldSettings>()?

Suggestion

To avoid confusion, I think we should:

  • Update the linked documentation comment.
  • Restore PopulateSettings, even just as a stub method that has [Obsolete] and throws NotSupportedException to aid migration.
  • Mention the removal of PopulateSettings in 1.9.0.md.
  • Replace definition.Settings.ToObject<T>() with definition.GetSettings<T>()?

If you have no objections, I will do the above and submit a PR in the coming days.

@Piedone
Copy link
Member

Piedone commented Mar 13, 2024

@MikeAlhayek could you chime in here, please?

@MikeAlhayek
Copy link
Member

I don't know the real reason why JT removed that extension.

Since 1.9 contains multiple breaking changes around the STJ conversion, I think it is okay to remove the extension method "unless we want to bring it back permanently without the use of Obsolete attribute". But, I also leaning toward not brining it back.

At the very least, the suggested documentation change should be part of the 1.9 breaking changes bullets in the STJ section as well. And the following line should be fixed accordingly

Do not access this property directly. Migrate to use GetSettings and PopulateSettings.

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 a pull request may close this issue.

4 participants