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

Replace all instances of BoM or Bom in class names with BOM #91

Closed
wants to merge 3 commits into from

Conversation

Andy-Grigg
Copy link
Collaborator

@Andy-Grigg Andy-Grigg commented Feb 1, 2022

This PR changes all instances of 'Bom' or 'BoM' in class names with 'BOM'.

This change is to ensure we are as compliant with PEP8 as is reasonable. Some considerations:

  • The Granta-preferred capitalization for the acronym is 'BoM'. PEP8 states that class names should use CapWords here, and that when using CapWords acronyms should be fully capitalized. Therefore, Bom1711..., BomImpactedSubstances..., etc. change to BOM1711..., BOMImpactedSubstances..., etc.
  • All references to these classes in docstrings and RST files have been changed
  • Any references to BoMs in method, module, and package names are unchanged, since they already were (and remain) fully lowercase
  • References to BoMs in docstrings (when not referring to a class) remain as 'BoM'
  • Classes in the imported ansys-grantami-bomanalytics-openapi package capitalize the 'B' only, this is due to the way the code generator template generates Python class names and cannot be changed without a great deal of effort

I did make a few changes to private docstrings, but these were just to standardize on 'BoM' when referring to the concept of a Bill of Materials, as opposed to 'bom'.

Copy link
Collaborator

@da1910 da1910 left a comment

Choose a reason for hiding this comment

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

One missing BOM, otherwise looks to do what it says on the tin.

I'm mostly 👎 to the change in general, but I do understand the logic following PEP-8

def create_definition(bom: str) -> BoM1711Definition:
"""Instantiate and return a ``Bom1711Definition`` object based on the provided bom.
def create_definition(bom: str) -> BOM1711Definition:
"""Instantiate and return a ``BoM1711Definition`` object based on the provided BoM.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missed a BOM here

@Andy-Grigg
Copy link
Collaborator Author

One missing BOM, otherwise looks to do what it says on the tin.

I'm mostly 👎 to the change in general, but I do understand the logic following PEP-8

I was trying to figure out when making this change if it's more or less readable than it was originally. I was also negative when making the change, but I put that down to my bias in having grown accustomed to the original class.

I suppose the debate is whether 'BoM' is really an acronym in the same way that 'HTTP' is (the example used in PEP8). I know BoM is an acronym, but it's vocalized as 'bomb', and so you're more likely to expect to see it as a regularly-cased CapWord string.

@Andy-Grigg
Copy link
Collaborator Author

I'm erring on the side of rejecting this PR. My reasoning is as follows:

  • Whilst PEP8 states that an acronym should be all-caps in class names, the cited example in PEP8 (HTTP) is an initialism, not an acronym
  • I believe the intent behind PEP8's stance is that if you have a class name based on an initialism, i.e. you say each letter separately (HTTP), then it should be capitalized.
  • This isn't the case for BoM/BOM, even though it's all caps it's pronounced as a word.

The current code is largely consistent with this. The one exception is in places where we refer to Granta-specific concepts such as 17/11 XML BoM formats, where we use the Granta capitalization of BoM.

As a result, I'll close this PR and delete the branch. I'll create an Issue that refers to it, copy this conclusion there, and close it.

@Andy-Grigg Andy-Grigg closed this Feb 2, 2022
@Andy-Grigg Andy-Grigg deleted the fix/PEP8-BOM branch February 2, 2022 15:55
@Andy-Grigg Andy-Grigg linked an issue Feb 2, 2022 that may be closed by this pull request
Andy-Grigg pushed a commit that referenced this pull request Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PEP8 compliance for acronyms in class names
2 participants