Skip to content

DOC: Avoid documenting ivar default values in SpatialFunction class#4115

Merged
jhlegarreta merged 1 commit intoInsightSoftwareConsortium:masterfrom
jhlegarreta:AvoidDocumentingIvarDefaultValueInConicShell
Jul 17, 2023
Merged

DOC: Avoid documenting ivar default values in SpatialFunction class#4115
jhlegarreta merged 1 commit intoInsightSoftwareConsortium:masterfrom
jhlegarreta:AvoidDocumentingIvarDefaultValueInConicShell

Conversation

@jhlegarreta
Copy link
Copy Markdown
Member

Avoid documenting ivar default values in
itk::ConicShellInteriorExteriorSpatialFunction since:

  • All ivar values should be initialized to some default value.
  • When default values are changed, it is easy to forget about updating the documentation and making it inconsistent with the actual value in the code.

PR Checklist

  • No API changes were made (or the changes have been approved)
  • No major design changes were made (or the changes have been approved)
  • Updated API documentation (or API not changed)

Avoid documenting ivar default values in
`itk::ConicShellInteriorExteriorSpatialFunction` since:
- All ivar values should be initialized to some default value.
- When default values are changed, it is easy to forget about updating
  the documentation and making it inconsistent with the actual value in
  the code.
@github-actions github-actions bot added type:Documentation Documentation improvement or change area:Core Issues affecting the Core module labels Jul 16, 2023
@jhlegarreta
Copy link
Copy Markdown
Member Author

jhlegarreta commented Jul 16, 2023

Not sure there is another way to show ivar default values through the Doxygen documentation, but I do think that we should avoid documenting explicitly for the above reasons.

Cross-referencing: InsightSoftwareConsortium/ITKSoftwareGuide#202

Copy link
Copy Markdown
Contributor

@N-Dekker N-Dekker left a comment

Choose a reason for hiding this comment

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

Well... in general there is always redundancy between documentation and implementation. And one may accidentally forget to keep the documentation consistent with the implementation. But in this particular case, the initialization is itself already included with the doxygen output at https://itk.org/Doxygen/html/classitk_1_1ConicShellInteriorExteriorSpatialFunction.html#aa0bf34461e6adf408abc7a0bf6e995c5 as it says:

bool [...] m_Polarity { false }

So end users can find the default initial value of Polarity from the documentation this way.

Thanks Jon! 👍

@jhlegarreta
Copy link
Copy Markdown
Member Author

Ah, thanks @N-Dekker, I had a quick look yesterday before adding the GitHub message and looks like I completely missed it.

@jhlegarreta jhlegarreta merged commit f602b3c into InsightSoftwareConsortium:master Jul 17, 2023
@jhlegarreta jhlegarreta deleted the AvoidDocumentingIvarDefaultValueInConicShell branch July 17, 2023 17:35
@jhlegarreta
Copy link
Copy Markdown
Member Author

Looks like the badges in the README and the Azure build status are not in sync. Not sure why. Badges are all green

itk_badges_20230718

but Azure builds are not

itk_azure_20230718

Also, badges are pointing at different commits across builds, e.g. f602b3c1 for ITK.Windows, 3c20912e for ITK.macOS.

@thewtex @tbirdso would you be able to investigate this, please? Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Core Issues affecting the Core module type:Documentation Documentation improvement or change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants