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.

Sign up for GitHub

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

Jump to bottom

Add LusasAdapterV191 #343

Merged
merged 7 commits into from
Jan 12, 2023
Merged

Add LusasAdapterV191 #343

merged 7 commits into from
Jan 12, 2023

Conversation

peterjamesnugent
Copy link
Member

@peterjamesnugent peterjamesnugent commented Aug 23, 2022 •
edited
Loading

Issues addressed by this PR

Closes #339

Test files

PushPullCompare script

Save results for LusasV191Adapter here

Compare to results for LusasV19Adapter here

Changelog

  • Added adapter for LusasV191
  • Changed type for .GetId() requests from Lusas to long to comply with Lusas19.1 updates and previous versions

Additional comments

You will need to build the Test_Toolkit for the push and pull script to work.

@peterjamesnugent peterjamesnugent added size:M Measured in hours type:external-api-changes Imposed changes, including from dependency across other BHoM repos priority:high High impact, high user value, driven by live project needs labels Aug 23, 2022
@peterjamesnugent peterjamesnugent added this to the BHoM 5.3 β MVP milestone Aug 23, 2022
@peterjamesnugent peterjamesnugent requested review from a user and StephennipBH August 23, 2022 10:31
@peterjamesnugent peterjamesnugent self-assigned this Aug 23, 2022
@ghost
Copy link

ghost commented Aug 23, 2022 •
edited by ghost
Loading

image
There are two different strategies happening here converting to an Int32 (int) or Int64 (long).
Two Questions:

  1. Would it not make sense to make all the Int32 as Int64's instead to future proof?
  2. Would it not make sense to keep everything as Int32, or even reducing them to Int16, in order to reduce the memory usage of the adapter?

@peterjamesnugent
Copy link
Member Author

Thanks @calumlockhart-bh, appears I missed a few and now updated.

I've added a test script to compare the functionality with the previous versions.

@ghost
Copy link

ghost commented Sep 2, 2022

Works fine in terms of pushing and pulling.

As mentioned above please add:
Number = Convert.ToInt32(Math.Max(Math.Min(adapterNameId, int32.MaxValue), int32.MinValue))

...or use a simple:
Number = unchecked((int)adapterNameId),
which is more efficient but defaults the value to 0 if it exceeds the maximum value for an int.

@FraserGreenroyd FraserGreenroyd added type:feature New capability or enhancement and removed type:external-api-changes Imposed changes, including from dependency across other BHoM repos labels Sep 2, 2022
@peterjamesnugent peterjamesnugent requested a review from a user September 28, 2022 15:27
@peterjamesnugent
Copy link
Member Author

@BHoMBot check required

@bhombot-ci
Copy link

bhombot-ci bot commented Sep 28, 2022

@peterjamesnugent 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

There are 63 requests in the queue ahead of you.

ghost
ghost previously approved these changes Sep 28, 2022
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Updated suggestion incorporated and happy with functionality from previous test.

@bhombot-ci
Copy link

bhombot-ci bot commented Jan 12, 2023

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

  • check ready-to-merge

There are 7 requests in the queue ahead of you.

@bhombot-ci
Copy link

bhombot-ci bot commented Jan 12, 2023

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

  • check ready-to-merge

There are 8 requests in the queue ahead of you.

@bhombot-ci
Copy link

bhombot-ci bot commented Jan 12, 2023

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

  • check ready-to-merge

There are 9 requests in the queue ahead of you.

@bhombot-ci
Copy link

bhombot-ci bot commented Jan 12, 2023

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

  • check ready-to-merge

There are 10 requests in the queue ahead of you.

@bhombot-ci
Copy link

bhombot-ci bot commented Jan 12, 2023

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

  • check ready-to-merge

There are 11 requests in the queue ahead of you.

@bhombot-ci
Copy link

bhombot-ci bot commented Jan 12, 2023

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

  • check ready-to-merge

There are 12 requests in the queue ahead of you.

@bhombot-ci
Copy link

bhombot-ci bot commented Jan 12, 2023

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

  • check ready-to-merge

There are 13 requests in the queue ahead of you.

@bhombot-ci
Copy link

bhombot-ci bot commented Jan 12, 2023

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

  • check ready-to-merge

There are 14 requests in the queue ahead of you.

@bhombot-ci
Copy link

bhombot-ci bot commented Jan 12, 2023

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

  • check ready-to-merge

There are 15 requests in the queue ahead of you.

@bhombot-ci
Copy link

bhombot-ci bot commented Jan 12, 2023

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

15 similar comments
@bhombot-ci
Copy link

bhombot-ci bot commented Jan 12, 2023

The check ready-to-merge 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 Jan 12, 2023

The check ready-to-merge 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 Jan 12, 2023

The check ready-to-merge 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 Jan 12, 2023

The check ready-to-merge 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 Jan 12, 2023

The check ready-to-merge 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 Jan 12, 2023

The check ready-to-merge 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 Jan 12, 2023

The check ready-to-merge 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 Jan 12, 2023

The check ready-to-merge 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 Jan 12, 2023

The check ready-to-merge 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 Jan 12, 2023

The check ready-to-merge 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 Jan 12, 2023

The check ready-to-merge 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 Jan 12, 2023

The check ready-to-merge 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 Jan 12, 2023

The check ready-to-merge 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 Jan 12, 2023

The check ready-to-merge 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 Jan 12, 2023

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

@FraserGreenroyd FraserGreenroyd merged commit 9aa3eeb into develop Jan 12, 2023
@FraserGreenroyd FraserGreenroyd deleted the Lusas_Toolkit-#339-UpgradeInteropForMinorVersion branch January 12, 2023 14:13
@bhombot-ci bhombot-ci bot mentioned this pull request Mar 13, 2023
Merged
@peterjamesnugent peterjamesnugent mentioned this pull request Jun 28, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StephennipBH StephennipBH StephennipBH approved these changes

@AntigoniChatzidaki AntigoniChatzidaki Awaiting requested review from AntigoniChatzidaki

Assignees

@peterjamesnugent peterjamesnugent

Labels
priority:high High impact, high user value, driven by live project needs size:M Measured in hours type:feature New capability or enhancement
Projects
None yet
Milestone
BHoM 6.0 β MVP
Development

Successfully merging this pull request may close these issues.

Upgrade introp for each minor version of Lusas
3 participants
@peterjamesnugent @StephennipBH @FraserGreenroyd