Remove function #2634
Remove function #2634
Conversation
anderson-joyle
force-pushed
the
andersonf/2623-remove-function
branch
from
bf65e7d
to
5979601
Compare
|
Public API changes: |
|
✅ No public API change. #Resolved |
anderson-joyle
force-pushed
the
andersonf/2623-remove-function
branch
from
f9bfa73
to
7b3d469
Compare
|
✅ No public API change. #Resolved |
|
✅ No public API change. #Resolved |
| } | ||
| } | ||
|
|
||
| if (context.AnalysisMode) |
There was a problem hiding this comment.
The original code wasn't checking if context.Features.PowerFxV1CompatibilityRules was active or not. The idea here is, whatever PA was returning, keep returning it. Since PFxV1 in Core is active, return Void.
There was a problem hiding this comment.
When Power Apps moves to PFxV1, it should have the same behavior as the C# interpreter. Remove is one of the functions that we will update the return type on V1 to be Void.
| errors.EnsureError(DocumentErrorSeverity.Severe, args[i], TexlStrings.ErrTableDoesNotAcceptThisType); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This check wasn't in the original Remove implementation; what is the scenario that it is covering? In the existing implementation, there is already a check for the types of the arguments compared to the data source / collection.
There was a problem hiding this comment.
My understanding is that the returning types are different between PA/PFx.
There was a problem hiding this comment.
If I understand correctly, this is not about the return type of the function, but for input validation. And as I mentioned in another comment, whenever we expose Power Fx V1 compatibility in Power Apps, it should have the same behavior as the C# interpreter.
| } | ||
|
|
||
| // Remove(collection:*[], source:*[], ["All"]) | ||
| internal class RemoveAllFunction : BuiltinFunction |
There was a problem hiding this comment.
Ah, I noticed the difference (record arguments vs. table arguments). It can probably be refactored, though, to a common base type, so that most shared code can be written once.
| { "First", "first" }, | ||
| { "All", "all" }, | ||
| }, | ||
| canCoerceFromBackingKind: true); |
There was a problem hiding this comment.
What about other enums that are also coercible (DateTimeFormatEnum)? I unmderstand we want to force makers to to write Remove.All but shouldn't we also take in consideration that this is still supported in PA?
There was a problem hiding this comment.
DateTimeFormat is an enum that is passed to the Text function - and it can also take a string value. This is more similar to something like SortOrder - there is only one possible value that can be used
| recordsToRemove = args | ||
| .Skip(1) | ||
| .Where(arg => arg is RecordValue) | ||
| .Select(row => (RecordValue)row) |
There was a problem hiding this comment.
.OfType<RecordValue>()
Not related to your changes, but can you change the file name from Remove_V1Compact.txt to Remove_V1Compat.txt? Refers to: src/tests/Microsoft.PowerFx.Core.Tests.Shared/ExpressionTestCases/Remove_V1Compact.txt:1 in f4ede39. [](commit_id = f4ede39, deletion_comment = False) |
The "base" remove test (Remove.txt) already uses the PowerFxV1CompatibilityRules setup; do we need this file, or can we merge it into the base one? Refers to: src/tests/Microsoft.PowerFx.Core.Tests.Shared/ExpressionTestCases/Remove_V1Compact.txt:1 in f4ede39. [](commit_id = f4ede39, deletion_comment = False) |
| @@ -164,7 +164,7 @@ public void DataTableEvalTest2() | |||
| var result7 = engine.Eval("Patch(robintable, First(robintable),{Names:\"new-name\"});robintable", options: opt); | |||
| Assert.Equal("Table({Names:\"new-name\",Scores:10},{Names:\"name3\",Scores:30},{Names:\"name100\",Scores:10})", ((DataTableValue)result7).Dump()); | |||
|
|
|||
| var result8 = engine.Eval("Remove(robintable, {Scores:10}, \"All\");robintable", options: opt); | |||
| var result8 = engine.Eval("Remove(robintable, {Scores:10}, RemoveFlags.All);robintable", options: opt); | |||
|
✅ No public API change. |
|
✅ No public API change. |
|
✅ No public API change. |
|
✅ No public API change. |
Unifying Remove function between PFx/PA.
Issue #2623.