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

Removed auto-constructors for finicky objects #135

Conversation

tg359
Copy link
Contributor

@tg359 tg359 commented Nov 13, 2023

NOTE: Depends on

Issues addressed by this PR

Closes #131

[NoAutoConstructor] added to objects where they are either created via a process, or have some validation required in order to be suitable for use in downstream processes.

Test files

Changelog

Additional comments

@tg359 tg359 self-assigned this Nov 13, 2023
@tg359 tg359 added severity:low Doesn't stop/slow current workflow size:S Measured in minutes labels Nov 13, 2023
Copy link
Contributor

@Tom-Kingstone Tom-Kingstone left a comment

Choose a reason for hiding this comment

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

Some minor changes needed

LadybugTools_Engine/Create/AnalysisPeriod.cs Outdated Show resolved Hide resolved
LadybugTools_Engine/Create/EnergyMaterial.cs Outdated Show resolved Hide resolved
LadybugTools_Engine/Create/EnergyMaterialVegetation.cs Outdated Show resolved Hide resolved
LadybugTools_Engine/Create/Location.cs Outdated Show resolved Hide resolved
LadybugTools_Engine/Create/Typology.cs Outdated Show resolved Hide resolved
LadybugTools_Engine/Create/AnalysisPeriod.cs Outdated Show resolved Hide resolved
LadybugTools_Engine/Create/EnergyMaterial.cs Outdated Show resolved Hide resolved
LadybugTools_Engine/Create/EnergyMaterial.cs Outdated Show resolved Hide resolved
LadybugTools_Engine/Create/EnergyMaterial.cs Outdated Show resolved Hide resolved
LadybugTools_Engine/Create/EnergyMaterial.cs Outdated Show resolved Hide resolved
LadybugTools_Engine/Create/EnergyMaterial.cs Outdated Show resolved Hide resolved
LadybugTools_Engine/Create/EnergyMaterial.cs Outdated Show resolved Hide resolved
LadybugTools_Engine/Create/EnergyMaterialVegetation.cs Outdated Show resolved Hide resolved
LadybugTools_Engine/Create/Location.cs Outdated Show resolved Hide resolved
LadybugTools_oM/EnergyMaterial.cs Show resolved Hide resolved
@tg359 tg359 requested a review from Tom-Kingstone November 13, 2023 14:01
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.

Project compliance will fail so let's get the bot to fix that after Typology.

@BHoMBot check project-compliance

@Tom-Kingstone
Copy link
Contributor

@BHoMBot check project-compliance

Copy link

bhombot-ci bot commented Nov 14, 2023

@Tom-Kingstone to confirm, the following actions are now queued:

  • check project-compliance

There are 11 requests in the queue ahead of you.

Copy link

bhombot-ci bot commented Nov 14, 2023

@FraserGreenroyd fix requested for project compliance.

The errors with the CSProject (.csproj) files have been recorded as annotations on the checks tab.

I will apply the fixes to every case detailed on the checks tab with the exception of any references to the target framework. I am unable to provide fixes to the Target Framework automatically, these will need to be performed manually. If you want to perform the fixes in a different manner please resolve this manually and rerun the check.

If you are happy for me to go ahead and perform this action, please reply with:

@BHoMBot fix project file ref. 18659371395

@FraserGreenroyd
Copy link
Contributor

@BHoMBot fix project file ref. 18659371395

Copy link

bhombot-ci bot commented Nov 14, 2023

@FraserGreenroyd I have queued up your request to fix the csproj file(s). There are 2 requests in the queue ahead of you.

Copy link

bhombot-ci bot commented Nov 14, 2023

@FraserGreenroyd I am now going to fix the project compliance in accordance with the annotations previously made.

Copy link

bhombot-ci bot commented Nov 14, 2023

@FraserGreenroyd to confirm I have now resolved the project compliance issues and pushed a commit to this Pull Request.

@FraserGreenroyd
Copy link
Contributor

@BHoMBot check compliance

Copy link

bhombot-ci bot commented Nov 14, 2023

@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 2 requests in the queue ahead of you.

@Tom-Kingstone Tom-Kingstone added the type:feature New capability or enhancement label Nov 14, 2023
@Tom-Kingstone
Copy link
Contributor

@BHoMBot check compliance
@BHoMBot check required

Copy link

bhombot-ci bot commented Nov 14, 2023

@Tom-Kingstone 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 code-compliance
  • check documentation-compliance
  • check project-compliance
  • check core
  • check null-handling
  • check serialisation
  • check versioning
  • check installer

There are 1 requests in the queue ahead of you.

Copy link

bhombot-ci bot commented Nov 14, 2023

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.

Copy link

bhombot-ci bot commented Nov 14, 2023

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

Copy link

bhombot-ci bot commented Nov 14, 2023

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.

All good on code review with comments resolved, thanks @Tom-Kingstone for picking up the additional NoAutoConstructor discussed with @tg359 on Teams.

Happy to deploy for further testing.

Tom-Kingstone
Tom-Kingstone previously approved these changes Nov 14, 2023
Copy link
Contributor

@Tom-Kingstone Tom-Kingstone left a comment

Choose a reason for hiding this comment

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

approved as @FraserGreenroyd will set out

@FraserGreenroyd FraserGreenroyd dismissed stale reviews from Tom-Kingstone and themself via b5bc6dc November 14, 2023 10:50
@FraserGreenroyd FraserGreenroyd force-pushed the LBT_Tk-#131-EnforceObjectConstructionViaUIsUsingCreateMethods branch from 0a8359f to b5bc6dc Compare November 14, 2023 10:50
@FraserGreenroyd
Copy link
Contributor

@BHoMBot check compliance
@BHoMBot check required

Copy link

bhombot-ci bot commented Nov 14, 2023

@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
  • check code-compliance
  • check documentation-compliance
  • check project-compliance
  • check core
  • check null-handling
  • check serialisation
  • check versioning
  • check installer

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.

Reapproving based on this comment following a rebase to resolve conflicts.

Copy link

bhombot-ci bot commented Nov 14, 2023

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.

Copy link

bhombot-ci bot commented Nov 14, 2023

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

bhombot-ci bot commented Nov 14, 2023

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

@FraserGreenroyd
Copy link
Contributor

@BHoMBot this is a DevOps instruction. I am requesting neutral checks on: installer, unit-tests, ready-to-merge

Copy link

bhombot-ci bot commented Nov 14, 2023

@FraserGreenroyd I have provided neutral checks to the checks requested. These checks will need to be run properly to obtain full results.

@FraserGreenroyd FraserGreenroyd merged commit af85473 into develop Nov 14, 2023
13 checks passed
@FraserGreenroyd FraserGreenroyd deleted the LBT_Tk-#131-EnforceObjectConstructionViaUIsUsingCreateMethods branch November 14, 2023 12:26
@bhombot-ci bhombot-ci bot mentioned this pull request Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
severity:low Doesn't stop/slow current workflow size:S Measured in minutes type:feature New capability or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hide object inits from the objects themselves and enforce creation via Create methods
4 participants