-
Notifications
You must be signed in to change notification settings - Fork 54
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
Idea for better (non-redundant, more compact) branching syntax for selectors? #1155
Comments
It's completely impossible because duplicate keys are forbidden :( |
Well, how about making each In other words, the following (note the extra build:
- match: build_platform != target_platform
# cross-compilation
- match: build_platform != target_platform and cuda_version
# cross-compiled CUDA
- match: match(cuda, "<12")
# CUDA 11.x
- match: match(cuda, ">=12.0")
# CUDA 12.x
- match: linux
# linux
- match: osx
# osx
- match: unix
# unix
- match: win
# win
- match: blas_impl == "mkl"
# mkl ... should be desugared to the same list as the impossible variant that has multiple |
Why not use a top level |
I'd be happy with that too! |
But how is it different from multiple if statements? |
You dont need to repeat the negation of the if clauses. You can more easily write something specific for win, macos, unix without having to repeat not win, etc. Sorry, im on a phone so its a bit hard to type a proper example. |
For the record, it's actually possible to use arbitrary Jinja in multi-line YAML strings. I am not sure if we want to keep it, but the following works (after a small bug fix from #1156): build:
script: |
echo "Is ${{ target_platform == "linux-64" }}"
{% if target_platform == "linux-64" %}
echo "I AM A LINUX"
{% elif osx %}
echo "I AM A MAC"
{% endif %} And of course, you could just as well use bash. So I am not sure if it's worth it to make our syntax more complicated or not ... Right now it's also possible to do arbitrary things with set and for loops btw. - it just has to be encapsulated in a multi-line string :) |
One of the design goals was to make the recipe easier to parse for machines. I think what you are suggesting is taking a step back. Having a match-kinda statement seems like a great (and logical) building block. |
Yeah but we've never semantically understood the contents of the scripts themselves so I'm not sure if we need that. This doesn't affect the "everything is pure YAML" property. |
Sure but just as you mentioned in another issue you just closed, we can validate the control flow statements using the schema. That would become harder and harder the more jinja we start using. |
If we build our own LSP though .. :) I think I do get your point, but I don't see it super different from e.g. people having issues with their bash scripts inside the script tag. |
This does not seem to work:
The issue with passing to a |
Where did you set But if you do:
It has a chance of working :) |
Actually, one reason why I do have a preference for bare |
@h-vetinari sorry, not following. What's the difference of
|
Also your initial example is not at all valid YAML if I see that correctly. |
Attached two screenshots highlighting the issues (from https://yamlchecker.com/) |
I explained why that's not desirable in the OP (or at least that there are cases where branches shouldn't be exclusive).
Yes, I hate YAML. 🙃 |
The way I understand it is that you want "if"-syntax without You want falsey branches to be skipped, but every truthy branch to be taken, right? In e.g. Rust, when usign But if I understand your proposal correctly, you want every truthy branch to "match"? Can you add corrected YAML to your proposal? Because I think if you write out proper YAML, it will look just like the current |
Right! Silly me, I forgot I needed to split (I did know that, just got caught in the Jinja mixin). The variable is in the For this particular case, it seems that the rattler/jinja is more complex. The array needs to be joined, then split in script where the {%%} is accepted. Am I missing something? Also, it seems that the comments are not completely ignored, I had mistakenly put a "xx' in a comment and it seemed to generate a syntax error, that went away after correction (not 100% sure, though) Update: Not to be difficult, but the initial point was to have the list be defined once for the several packages that use it. I don't think it will work:
So I will use |
While thinking about #1154, it occurred to me that it would be even nicer if we could do a match-style syntax that doesn't have to choose a single branch but could apply several!
I guess it might be a bit late for such deep syntactic changes, but IMO this would make it much nicer to write recipes, rather than having to constantly restart
if:
branches to avoid exhaustivity-problems.It would allow writing using a single
match:
(or whatever you want to call it, like the oldsel:
before it turned intoif:
) block for every section of the recipe, which would be much nicer IMO if we're looking at translating things likeexamples
this
or this
etc.
The more dimensions (unix vs win, native vs. cross, python version, CUDA or not, implementation variants for openmpi/BLAS/QT/etc.) we need to cover in a given recipe, the harder it gets to do that with simple
if: else:
branches. Consider something likeIt would be a much lower mental overhead to be able to freely shuffle around lines based on which sections they apply to (e.g. not having to restructure the following into correctly (non-)overlapping
if:
statements), only adding lines to the selector where they are supposed to be applied.The text was updated successfully, but these errors were encountered: