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

Add adapter module for fetching dependency objects #305

Conversation

IsakNaslundBh
Copy link
Contributor

Issues addressed by this PR

Closes #304

Adds AdapterModule for fetching dependency objects for edgecases where the method in Reflection_Engine can not handle it.

First usecase added for getting Loadcases from LoadCombinations

Test files

Changelog

Additional comments

@IsakNaslundBh IsakNaslundBh added the type:feature New capability or enhancement label Aug 24, 2021
@IsakNaslundBh IsakNaslundBh self-assigned this Aug 24, 2021
@IsakNaslundBh IsakNaslundBh added this to In Progress in SCRUM Pull Request Tracker via automation Aug 24, 2021
@IsakNaslundBh
Copy link
Contributor Author

@BHoMBot check compliance
@BHoMBot check required

@bhombot-ci
Copy link

bhombot-ci bot commented Aug 24, 2021

@IsakNaslundBh to confirm, the following checks are now queued:

  • code-compliance
  • documentation-compliance
  • project-compliance
  • branch-compliance
  • dataset-compliance
  • copyright-compliance
  • code-compliance
  • documentation-compliance
  • project-compliance
  • core
  • null-handling
  • serialisation
  • installer
  • versioning

@bhombot-ci
Copy link

bhombot-ci bot commented Aug 24, 2021

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 Aug 24, 2021

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.

@bhombot-ci
Copy link

bhombot-ci bot commented Aug 24, 2021

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.

@IsakNaslundBh
Copy link
Contributor Author

@BHoMBot check compliance
@BHoMBot check required

@bhombot-ci
Copy link

bhombot-ci bot commented Aug 24, 2021

@IsakNaslundBh to confirm, the following checks are now queued:

  • code-compliance
  • documentation-compliance
  • project-compliance
  • branch-compliance
  • dataset-compliance
  • copyright-compliance
  • code-compliance
  • documentation-compliance
  • project-compliance
  • core
  • null-handling
  • serialisation
  • installer
  • versioning

There are 10 requests in the queue ahead of you.

@IsakNaslundBh IsakNaslundBh changed the title B ho m adapter #304 add adapter module for fetching dependency objects Add adapter module for fetching dependency objects Aug 25, 2021
@JosefTaylor JosefTaylor self-requested a review September 14, 2021 15:49
JosefTaylor
JosefTaylor previously approved these changes Sep 14, 2021
Copy link
Contributor

@JosefTaylor JosefTaylor left a comment

Choose a reason for hiding this comment

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

works flawlessly on LoadCombinations, excellent. Made a commit suggestion for a typo.

Adapter_oM/Module/IGetDependencyModule.cs Outdated Show resolved Hide resolved
SCRUM Pull Request Tracker automation moved this from In Progress to Reviewer approved Sep 14, 2021
SCRUM Pull Request Tracker automation moved this from Reviewer approved to Review in Progress Sep 15, 2021
@alelom
Copy link
Member

alelom commented Sep 15, 2021

The change makes sense to me. However, I'm considering whether we could revise the GetDependencyObjects() signature.

The inner logic effectively makes the adapter parameter optional, as the method proceeds whether it finds any module or not.
Therefore I would either:

  • refactor the signature to be something like
GetDependencyObjects<T>(IEnumerable<T> objects, List<Type> dependencyTypes, IBHoMAdapter adapter = null)
  • or, alternatively, create two methods, one with the additional BHoMAdapter parameter and one without (with the old functionality), where one can call the other.

I just think this would make things clearer; even though one method would probably rarely be used, we still should consider that this method is publicly available as part of an Engine. What do you think @IsakNaslundBh ?

@IsakNaslundBh
Copy link
Contributor Author

I just think this would make things clearer; even though one method would probably rarely be used, we still should consider that this method is publicly available as part of an Engine. What do you think @IsakNaslundBh ?

Happy to change to that!

Also, as this can not go in until next milestone, would not mind to have a quick brainstorm call with you about this in general, and maybe iron out a few things here before merge anyway :)

@alelom
Copy link
Member

alelom commented Sep 15, 2021

would not mind to have a quick brainstorm call with you about this in general, and maybe iron out a few things here before merge anyway

sounds great!

@IsakNaslundBh
Copy link
Contributor Author

@BHoMBot check core

@IsakNaslundBh IsakNaslundBh force-pushed the BHoM_Adapter-#304-AddAdapterModuleForFetchingDependencyObjects branch from d647d05 to 45f6c68 Compare October 6, 2021 12:23
@IsakNaslundBh
Copy link
Contributor Author

@JosefTaylor @alelom I had to rebase and forcepush an update to this PR due to the framework update PR #308 that was merged last week. To be able to retest this, please ensure to delete the branch locally and pull it again.

Copy link
Member

@alelom alelom left a comment

Choose a reason for hiding this comment

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

The changes make sense to me. Existing functionality is kept. LGTM.

SCRUM Pull Request Tracker automation moved this from Review in Progress to Reviewer approved Oct 6, 2021
@IsakNaslundBh
Copy link
Contributor Author

@BHoMBot check compliance
@BHoMBot check required

@bhombot-ci
Copy link

bhombot-ci bot commented Oct 7, 2021

@IsakNaslundBh to confirm, the following checks are now queued:

  • code-compliance
  • documentation-compliance
  • project-compliance
  • branch-compliance
  • dataset-compliance
  • copyright-compliance
  • code-compliance
  • documentation-compliance
  • project-compliance
  • core
  • null-handling
  • serialisation
  • installer
  • versioning

@bhombot-ci
Copy link

bhombot-ci bot commented Oct 7, 2021

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 7, 2021

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.

@bhombot-ci
Copy link

bhombot-ci bot commented Oct 7, 2021

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.

@IsakNaslundBh
Copy link
Contributor Author

@BHoMBot check ready-to-merge

@bhombot-ci
Copy link

bhombot-ci bot commented Oct 7, 2021

@IsakNaslundBh to confirm, the following checks are now queued:

  • ready-to-merge

There are 12 requests in the queue ahead of you.

@IsakNaslundBh IsakNaslundBh merged commit 7ef2da3 into main Oct 7, 2021
@IsakNaslundBh IsakNaslundBh deleted the BHoM_Adapter-#304-AddAdapterModuleForFetchingDependencyObjects branch October 7, 2021 09:04
SCRUM Pull Request Tracker automation moved this from Reviewer approved to Completed Oct 7, 2021
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
Development

Successfully merging this pull request may close these issues.

Add module type for fetching dependencies
3 participants