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

Initial commit #3

Merged
merged 33 commits into from
Nov 11, 2019
Merged

Initial commit #3

merged 33 commits into from
Nov 11, 2019

Conversation

marco-bertschi
Copy link
Contributor

@marco-bertschi marco-bertschi commented Oct 31, 2019

First and initial commit to port our BBT structure tools to GitHub.

Copy link
Collaborator

@pascalberger pascalberger left a comment

Choose a reason for hiding this comment

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

@marco-bertschi I had a quick look, without digging into the functionality. Some points raised, see comments.

setup.cake Outdated Show resolved Hide resolved
setup.cake Outdated Show resolved Hide resolved
src/BBT.StructureTools/BBT.StructureTools.csproj Outdated Show resolved Hide resolved
src/BBT.StructureTools/Compare/IComparer.cs Outdated Show resolved Hide resolved
Copy link
Collaborator

@pascalberger pascalberger left a comment

Choose a reason for hiding this comment

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

Some more points added.

Also public API is quite huge. Does this stuff really all need to be public (e.g. BBT.StructureTools.Compare.Helper)? Can you please go through the API and make everything internal which doesn't need to be public (you can call .\build.ps1 -target preview-documentation and check in the documentation the public API).

src/BBT.StructureTools/Extension/BitOperations.cs Outdated Show resolved Hide resolved
src/BBT.StructureTools/Extension/LookupUtils.cs Outdated Show resolved Hide resolved
src/BBT.StructureTools/Initialization/IIocResolver.cs Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Nov 1, 2019

Codecov Report

❗ No coverage uploaded for pull request base (develop@b975c0a). Click here to learn what that means.
The diff coverage is 43.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop       #3   +/-   ##
==========================================
  Coverage           ?   43.75%           
==========================================
  Files              ?      100           
  Lines              ?     1826           
  Branches           ?      128           
==========================================
  Hits               ?      799           
  Misses             ?     1003           
  Partials           ?       24
Impacted Files Coverage Δ
...ateToManyWithGenericStrategyReverseRelationOnly.cs 0% <0%> (ø)
...reTools/Convert/Strategy/OperationConvertToMany.cs 0% <0%> (ø)
...ategy/OperationCopyOneToManyWithReverseRelation.cs 0% <0%> (ø)
.../BBT.StructureTools/CopyConvertCompareException.cs 0% <0%> (ø)
...ctureTools/Convert/Strategy/OperationMergeLevel.cs 0% <0%> (ø)
...tructureTools/Convert/Strategy/OperationSubCopy.cs 0% <0%> (ø)
src/BBT.StructureTools/Compare/ModelIntComparer.cs 0% <0%> (ø)
...ureTools/Convert/Value/ConvertValueRegistration.cs 0% <0%> (ø)
...eTools/Convert/Strategy/ConvertStrategyProvider.cs 0% <0%> (ø)
...tructureTools/Convert/Value/ValueConvertMapping.cs 0% <0%> (ø)
... and 90 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b975c0a...1bf5d0e. Read the comment docs.

Copy link
Contributor Author

@marco-bertschi marco-bertschi left a comment

Choose a reason for hiding this comment

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

@pascalberger

Some more points added.

Also public API is quite huge. Does this stuff really all need to be public (e.g. BBT.StructureTools.Compare.Helper)? Can you please go through the API and make everything internal which doesn't need to be public (you can call .\build.ps1 -target preview-documentation and check in the documentation the public API).

I'll do that sunday. I've fixed your other comments already. Can you have an extra look at the nuspec file?

@Speeedy01
Copy link
Contributor

@marco-bertschi let me know if you need a review or @pascalberger will do it?

@pascalberger
Copy link
Collaborator

@Speeedy01 I only did have a quick look without reviewing the business logic of the code. Happy to have you do another review

Copy link
Contributor

@Speeedy01 Speeedy01 left a comment

Choose a reason for hiding this comment

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

Small changes and suggestions

@marco-bertschi marco-bertschi dismissed pascalberger’s stale review November 11, 2019 07:15

Requested changes were addressed

@marco-bertschi marco-bertschi merged commit 81da861 into develop Nov 11, 2019
@marco-bertschi marco-bertschi deleted the topic/hir/InitialMovement branch November 11, 2019 07:16
@pascalberger pascalberger mentioned this pull request Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants