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

BHoM_Engine: Add DisplayText support on enums #2930

Merged
merged 3 commits into from
Oct 20, 2022

Conversation

adecler
Copy link
Member

@adecler adecler commented Oct 20, 2022

NOTE: Depends on

BHoM/BHoM#1441

Issues addressed by this PR

See BHoM/BHoM#1440

Test files

Not a file per se since there isn't obviously a case of DisplayText usage in the BHoM yet. But here's a working example:

image

image

Best is to add the attribute yourself to a random enum and test that it is well supported. Happy to jump to a call if it is easier for the review though.

@adecler adecler added the type:feature New capability or enhancement label Oct 20, 2022
@adecler adecler self-assigned this Oct 20, 2022
@adecler adecler requested a review from al-fisher as a code owner October 20, 2022 13:19
@adecler
Copy link
Member Author

adecler commented Oct 20, 2022

@BHoMBot check compliance
@BHoMBot check versioning

@bhombot-ci
Copy link

bhombot-ci bot commented Oct 20, 2022

@adecler to confirm, the following actions are now queued:

  • check code-compliance
  • check documentation-compliance
  • check project-compliance
  • check branch-compliance
  • check dataset-compliance
  • check copyright-compliance
  • check versioning

There are 29 requests in the queue ahead of you.

Copy link
Contributor

@FraserGreenroyd FraserGreenroyd left a comment

Choose a reason for hiding this comment

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

image
Quick comment on an initial test @adecler - are we happy with the DisplayText value being displayed in the One Locally Defined Value area at the bottom?

Personally I am (because also I would expect people will put more appropriate display text items rather than what I've tested with) but just thought I'd raise it as a question while I test

@FraserGreenroyd
Copy link
Contributor

@BHoMBot check compliance

@bhombot-ci
Copy link

bhombot-ci bot commented Oct 20, 2022

@FraserGreenroyd to confirm, the following actions are now queued:

  • check code-compliance
  • check documentation-compliance
  • check project-compliance
  • check branch-compliance
  • check dataset-compliance
  • check copyright-compliance

There are 14 requests in the queue ahead of you.

@adecler
Copy link
Member Author

adecler commented Oct 20, 2022

image Quick comment on an initial test @adecler - are we happy with the DisplayText value being displayed in the One Locally Defined Value area at the bottom?

Personally I am (because also I would expect people will put more appropriate display text items rather than what I've tested with) but just thought I'd raise it as a question while I test

Yes, this is exactly what the ToText methods are for. Great to see it already is being used on a wider scope. Ideally we would always like to see enum displayed in a human readable way and never show the code version if there is a DisplayText attribute present. looks like the dropdown itself need to be updated though to is uses the DisplayText too. But one thing at a time 😄

@IsakNaslundBh
Copy link
Contributor

image Quick comment on an initial test @adecler - are we happy with the DisplayText value being displayed in the One Locally Defined Value area at the bottom?
Personally I am (because also I would expect people will put more appropriate display text items rather than what I've tested with) but just thought I'd raise it as a question while I test

Yes, this is exactly what the ToText methods are for. Great to see it already is being used on a wider scope. Ideally we would always like to see enum displayed in a human readable way and never show the code version if there is a DisplayText attribute present. looks like the dropdown itself need to be updated though to is uses the DisplayText too. But one thing at a time 😄

A quick question on how this DisplayText attribute is to be used compared to the Description attribute (that already has support for in the menu in #1586 .

Assume the DisplayText is meant to be an alias, as a short alternative name (allowing white space etc)? If so I can see a use for both. Just wanted to make sure there is not to much overlap :)

@adecler
Copy link
Member Author

adecler commented Oct 20, 2022

@IsakNaslundBh , check teh description of the issue: BHoM/BHoM#1440
You'll see that people are already using teh Description attribute for actual (longer) descriptions. So this serves the different purpose of readability in the UIs / web tools.

@adecler
Copy link
Member Author

adecler commented Oct 20, 2022

@BHoMBot check required

@bhombot-ci
Copy link

bhombot-ci bot commented Oct 20, 2022

@adecler to confirm, the following actions are now queued:

  • check code-compliance
  • check documentation-compliance
  • check project-compliance
  • check core
  • check null-handling
  • check serialisation
  • check versioning
  • check installer

@bhombot-ci
Copy link

bhombot-ci bot commented Oct 20, 2022

The check code-compliance has already been run previously and recorded as a successful check. This check has not been run again at this time.

@bhombot-ci
Copy link

bhombot-ci bot commented Oct 20, 2022

The check documentation-compliance has already been run previously and recorded as a successful check. This check has not been run again at this time.

Copy link
Contributor

@FraserGreenroyd FraserGreenroyd left a comment

Choose a reason for hiding this comment

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

I have tested both PRs by adding the new attribute to a couple of enum attributes and it is displaying well, in combination with displaying the default values where the attribute does not exist.

I am happy to see this merged, and am happy for now to not have unit tests for the methods on the basis that we currently do not have any enum values implementing the attribute in the oM - when that changes we will then want to add some unit tests for this method for the future.

@adecler
Copy link
Member Author

adecler commented Oct 20, 2022

I have tested both PRs by adding the new attribute to a couple of enum attributes and it is displaying well, in combination with displaying the default values where the attribute does not exist.

I am happy to see this merged, and am happy for now to not have unit tests for the methods on the basis that we currently do not have any enum values implementing the attribute in the oM - when that changes we will then want to add some unit tests for this method for the future.

Thanks @FraserGreenroyd . Yes, I agree on unit tests. They should be plenty of opportunities for the DisplaytText attribute within the BHoM already, so we should probably cover that within a wider exercise of improving enums throughout the BHoM. A frequent complain from reviewers I received when I started doing zero-code tools was that the enums where 'not displaying properly' in the interface (weird how people don't seem to like camel casing inside a UI 😄 ). So I am sure we need to do a similar exercise for our local BHoM UIs.

@FraserGreenroyd
Copy link
Contributor

@BHoMBot check installer

@bhombot-ci
Copy link

bhombot-ci bot commented Oct 20, 2022

@FraserGreenroyd to confirm, the following actions are now queued:

  • check installer

There are 49 requests in the queue ahead of you.

@adecler
Copy link
Member Author

adecler commented Oct 20, 2022

@BHoMBot check unit-tests
@BHoMBot check ready-to-merge

@bhombot-ci
Copy link

bhombot-ci bot commented Oct 20, 2022

@adecler to confirm, the following actions are now queued:

  • check unit-tests
  • check ready-to-merge

@adecler adecler merged commit 9e76399 into main Oct 20, 2022
@adecler adecler deleted the BHoM-#1440-DisplayTextForEnums branch October 20, 2022 16:50
@FraserGreenroyd FraserGreenroyd changed the title Add DisplayText support on enums BHoM_Engine: Add DisplayText support on enums Oct 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature New capability or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants