Misc fixes in the pivot functionallity #216
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Added `VariablePlacementTests` class in `VariablePlacementTests.cs` to test `PlacementHandler`. - Introduced `PlacementHandler` class in `PlacementHandler.cs` implementing `IPlacementHandler`. - Added `IPlacementHandler` interface in `IPlacementHandler.cs`. - Updated `ModelStore.cs` with `CreateModelA` method for sample `PXModel`. - Added `CommaSeparatedStringToListOfStrings` model binder in `CommaSeparatedStringToListOfStrings.cs`. - Modified `TableApiController.cs` to: * Inject `IPlacementHandler` dependency. * Use `CommaSeparatedStringToListOfStrings` model binder for `heading` and `stub` query parameters. * Replace inline placement logic with `PlacementHandler`. - Added `IllegalPlacementSelection` method in `ProblemUtility.cs`. - Registered `IPlacementHandler` service in `Program.cs`.
In `PlacementHandler.cs`: - Changed method to find the first time variable from `FirstOrDefault` to `Find`. - Updated method to check if all variables are in the model from using `All` to `TrueForAll`. In `CommaSeparatedStringToListOfStrings.cs`: - Simplified retrieval of query keys by removing unnecessary list conversions. - Removed null key check and moved query value retrieval outside the null check. - Refactored splitting of query values into items for conciseness. - Extracted logic for cleaning and adding items to the result list into a new private method `CleanValue`. - `CleanValue` method now trims items and removes surrounding square brackets if present.
Encapsulated the logic for replacing the "TIME" constant with the actual time code into a new private static method `ReplaceTimeConstant`. This method improves code readability and maintainability by handling the replacement within a dedicated function. The original inline code has been removed and replaced with a call to this new method.
Extracted complex conditional checks into new methods `AllVariablesExistsInSelection` and `OnlyHeadOrStubIsSpecified` to improve readability and maintainability. Replaced inline conditions with method calls. Moved `ReplaceTimeConstant` method to accommodate new methods.
Reordered parameters in `Assert.AreEqual` calls in `VariablePlacementTests.cs` to place the expected value first and the actual value second. This change improves readability and consistency, aligning with common unit testing practices.
| string? q = bindingContext.HttpContext.Request.Query[key]; | ||
| if (q != null) | ||
| { | ||
| var items = Regex.Split(q, ",(?=[^\\]]*(?:\\[|$))", RegexOptions.IgnoreCase, |
There was a problem hiding this comment.
Should we add try catch around the timeout add log something clever :-)
? Why IgnoreCase :-) ?
There was a problem hiding this comment.
I think that we can let code higher up in the chain catch the error. I Changed to ReqexOptions.None instead no need for case insensitivity.
| { | ||
| return value.Substring(1, item2.Length - 2); | ||
| } | ||
| return item2; |
There was a problem hiding this comment.
Should "return value.Substring(1, item2.Length - 2);" also have a Trim() ?
There was a problem hiding this comment.
good point, I will add that
- Introduced BindModelAsync method to handle model binding asynchronously, with a check for null bindingContext. - Changed RegexOptions in Regex.Split from IgnoreCase to None for case-sensitive splitting. - Added a foreach loop to clean and process each item in the split query string. - Enhanced CleanValue method to trim leading and trailing whitespace from cleaned substrings.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TableApiController.csto:IPlacementHandlerdependency.CommaSeparatedStringToListOfStringsmodel binder forheadingandstubquery parameters.PlacementHandler.VariablePlacementTestsclass inVariablePlacementTests.csto testPlacementHandler.PlacementHandlerclass inPlacementHandler.csimplementingIPlacementHandler.IPlacementHandlerinterface inIPlacementHandler.cs.ModelStore.cswithCreateModelAmethod for samplePXModel.CommaSeparatedStringToListOfStringsmodel binder inCommaSeparatedStringToListOfStrings.cs.IllegalPlacementSelectionmethod inProblemUtility.cs.IPlacementHandlerservice inProgram.cs.