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

Revit_Engine: add Revit-specific Diffing method #1106

Merged
merged 29 commits into from
Oct 13, 2021

Conversation

alelom
Copy link
Member

@alelom alelom commented Oct 6, 2021

NOTE: Depends on

BHoM/BHoM_Engine#2647

Issues addressed by this PR

Closes #1105

Added a Diffing() method in Revit_Engine that simply does some needed objects pre-processing before calling BH.Engine.Diffing.DiffWithFragmentId(), which does the actual diffing.

This pre-processing simply allows the Revit Parameters to be considered as properties of the object. Hence, an user of the diffing can specify Revit parameter names into DiffingConfig.PropertiesToConsider or DiffingConfig.PropertyExceptions as they were properties.

This method is also automatically invoked by the base method IDiffing if objects from the Revit namespace are detected.

Test files

https://burohappold.sharepoint.com/:f:/s/BHoM/Enre0HWKOkBHm-eUfHv9z5ABgzfiWOI-25ZZf9pPvZpnfg?e=G6jXA6

Changelog

Additional comments

alelom added 3 commits October 7, 2021 09:23
This test code is still useful to keep on branch as it may turn useful in the future. It was a fully working solution, only less elegant than the current one that leverages delegates and reflection.
Copy link
Member

@pawelbaran pawelbaran left a comment

Choose a reason for hiding this comment

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

By reading the code I have only found minor issues (see the comments), plus the compliance check will fail for sure.

On top of that, I cannot test the PR without a Revit file to pull from: it would be good to have a general example, plus possibly one with pulled parameter name equal to BHoM object property name - which will then be used for diffing?

Revit_Engine/Compute/Diffing.cs Outdated Show resolved Hide resolved
Revit_Engine/Compute/Diffing.cs Outdated Show resolved Hide resolved
Revit_Engine/Compute/Diffing.cs Outdated Show resolved Hide resolved
Revit_Engine/Compute/Diffing.cs Outdated Show resolved Hide resolved
@alelom
Copy link
Member Author

alelom commented Oct 8, 2021

On top of that, I cannot test the PR without a Revit file to pull from

As from our chat, there is Revit file provided (actually 2), see PR Test Files section.

Fixed the rest of changes requested.

@alelom alelom requested a review from pawelbaran October 8, 2021 13:03
@pawelbaran
Copy link
Member

@BHoMBot check compliance
@BHoMBot check null-handling

@bhombot-ci
Copy link

bhombot-ci bot commented Oct 11, 2021

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

  • code-compliance
  • documentation-compliance
  • project-compliance
  • branch-compliance
  • dataset-compliance
  • copyright-compliance
  • null-handling

@bhombot-ci
Copy link

bhombot-ci bot commented Oct 13, 2021

@alelom just to let you know, I have provided a check-installer result to this Pull Request as it was detected to be linked to other Pull Requests in a series. The comment which triggered this check came from @alelom on BHoM_Engine

@bhombot-ci
Copy link

bhombot-ci bot commented Oct 13, 2021

@alelom just to let you know, I have provided a check-versioning result to this Pull Request as it was detected to be linked to other Pull Requests in a series. The comment which triggered this check came from @alelom on BHoM_Engine

@alelom
Copy link
Member Author

alelom commented Oct 13, 2021

@BHoMBot check required

@bhombot-ci
Copy link

bhombot-ci bot commented Oct 13, 2021

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

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

There are 32 requests in the queue ahead of you.

@bhombot-ci
Copy link

bhombot-ci bot commented Oct 13, 2021

@alelom just to let you know, I have provided a check-versioning result to this Pull Request as it was detected to be linked to other Pull Requests in a series. The comment which triggered this check came from @alelom on BHoM_Engine

@bhombot-ci
Copy link

bhombot-ci bot commented Oct 13, 2021

@alelom 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. 3882445320

@alelom
Copy link
Member Author

alelom commented Oct 13, 2021

@BHoMBot check null-handling

@bhombot-ci
Copy link

bhombot-ci bot commented Oct 13, 2021

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

  • null-handling

@alelom
Copy link
Member Author

alelom commented Oct 13, 2021

@BHoMBot check null-handling

@bhombot-ci
Copy link

bhombot-ci bot commented Oct 13, 2021

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

  • null-handling

@alelom
Copy link
Member Author

alelom commented Oct 13, 2021

@BHoMBot check required

@bhombot-ci
Copy link

bhombot-ci bot commented Oct 13, 2021

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

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

@alelom alelom requested a review from pawelbaran October 13, 2021 16:44
Copy link
Member

@pawelbaran pawelbaran left a comment

Choose a reason for hiding this comment

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

Re-re-approving - comments as before 😉

@bhombot-ci
Copy link

bhombot-ci bot commented Oct 13, 2021

FAO: @FraserGreenroyd
@FraserGreenroyd is seeking dispensation on this Pull Request to skip a required check. Please can you provide authorisation for the check to be skipped, or provide assistance as appropriate.

The check they wish to have dispensation on is project-compliance.

If you are providing dispensation on this occasion, please reply with:

@BHoMBot this is a CI/CD instruction. I am authorising dispensation to be granted on check ref. 3885280773

@FraserGreenroyd
Copy link
Contributor

@BHoMBot this is a CI/CD instruction. I am authorising dispensation to be granted on check ref. 3885280773

@bhombot-ci
Copy link

bhombot-ci bot commented Oct 13, 2021

@FraserGreenroyd I have now provided a passing check on reference 3885280773 as requested.

@alelom
Copy link
Member Author

alelom commented Oct 13, 2021

@BHoMBot check ready-to-merge

@bhombot-ci
Copy link

bhombot-ci bot commented Oct 13, 2021

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

  • ready-to-merge

@FraserGreenroyd
Copy link
Contributor

@BHoMBot check copyright-compliance
@BHoMBot check dataset-compliance

@bhombot-ci
Copy link

bhombot-ci bot commented Oct 13, 2021

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

  • copyright-compliance
  • dataset-compliance

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.

Revit_Engine: add revit-specific Diffing method and simplify workflow
4 participants