Fixes filters using an aliased value#2615
Conversation
d3d0158 to
12087d6
Compare
12087d6 to
fb0f79b
Compare
| function suite.targetWithArm64FilterSuffix() | ||
| architecture "ARM64" | ||
|
|
||
| filter { "architecture:ARM64" } -- Should match when architecture is AARCH64 |
There was a problem hiding this comment.
Would it be worth adding a lowercase test?
There was a problem hiding this comment.
Or better yet, a mixed case test, e.g. aRm64.
There was a problem hiding this comment.
We probably should, but that sort of test would be better fit elsewhere in the filter logic, not the VS logic.
| x86_64 = "x64", | ||
| ARM = "ARM", | ||
| AARCH64 = "ARM64", | ||
| } |
There was a problem hiding this comment.
It is now identical to vs200x_architectures.
It seems they can be regrouped, and so below function might be simplified.
There was a problem hiding this comment.
Keeping is separate in case any plugins are relying on this.
src/base/context.lua
Outdated
| local field = p.field.get(key) | ||
| if field and field.aliases then | ||
| local aliased = field.aliases[value] | ||
| if aliased then | ||
| value = tostring(aliased):lower() | ||
| end | ||
| end |
There was a problem hiding this comment.
That pattern is repeated 3 times, maybe create a subfunction, something like:
function p.field.alias_resolved(key, value) do
value = value:lower()
local field = p.field.get(key)
if field and field.aliases then
local aliased = field.aliases[value]
if aliased then
value = tostring(aliased):lower()
end
end
return value
endThere was a problem hiding this comment.
These aren't quite the same. One is iterating over a table, one is a single value.
There was a problem hiding this comment.
The pattern is identical in all three places, and can be factored to call the function Jarod42 suggested.
Here is what the 3 call sites would become:
(Edit 1: fixed . to : for calling the function.)
(Edit 2: oops, Jarod42 already handled the missing :lower() issue in criteria.lua and factored further than I had initially noticed, and I've updated the call sites below accordingly.)
context.lua
if type(value) == "table" then
for i = 1, #value do
value[i] = p.field:alias_resolved(key, value[i])
end
elseif value ~= nil then
value = p.field:alias_resolved(key, value)
endcriteria.lua
if prefix then
word[1] = p.field:alias_resolved(prefix, word[1])
endThere was a problem hiding this comment.
Ah, misunderstood the comment
There was a problem hiding this comment.
LGTM! (and you're right, my . was right the first time, not :)
fb0f79b to
053d276
Compare
chrisant996
left a comment
There was a problem hiding this comment.
Looks good to me, thanks for addressing this issue!
What does this PR do?
Fixes #2614
How does this PR change Premake's behavior?
No
Anything else we should know?
N/A
Did you check all the boxes?
closes #XXXXin comment to auto-close issue when PR is merged)You can now support Premake on our OpenCollective. Your contributions help us spend more time responding to requests like these!