-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Model enhancement bugfix #3247
Model enhancement bugfix #3247
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3247 +/- ##
============================================
- Coverage 92.89% 92.88% -0.01%
Complexity 3523 3523
============================================
Files 382 382
Lines 9373 9348 -25
Branches 775 780 +5
============================================
- Hits 8707 8683 -24
+ Misses 477 476 -1
Partials 189 189
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formatting changes seems to be intermixed with coding changes. So its hard to review what has changed.
I guess the tests are passing, so I'll reformat and merge it in. We should talk about how to share code/formatting.
Map<String, Model> currentBranch, | ||
Map<String, ModelContext> contextMap) { | ||
public MergingContext(String rootId, Map<String, Set<Model>> typedModelMap, Map<String, String> modelIdToParameterId, | ||
Map<String, Model> currentBranch, Map<String, ModelContext> contextMap) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We dont want to arrange parameters in this form, because it makes it very difficult to find out what changed. In general prefer, one parameter per line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dilipkrish , I split refactoring and fixes in a few separated commits. Ok, we should discuss formatting and code style. As for me each parameter for a new line will make class file longer. That is sometimes not convenient to debug it.
this.contextMap, | ||
this.seenModels); | ||
return new MergingContext(this.rootId, parametersMatching, dependencies, comparisonConditions, this.typedModelMap, | ||
this.modelIdToParameterId, this.currentBranch, this.contextMap, this.seenModels); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also here, we want one parameter per line
|
||
import static springfox.documentation.schema.ResolvedTypes.*; | ||
import static springfox.documentation.schema.ResolvedTypes.modelRefFactory; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general prefer the splatting of static imports.
@MaksimOrlov thank you! |
That PR fixes issues after introducing new merging algorithm.
#3014, #3243, #2345, #3211, #3082, #563.
The main issue is when we faced model with nodes (properties) that we already merged, we just skip that model instead of looking for the same models of that type.