https://pantsbuild.org/ logo
Join Slack
Powered by
cont. <https://pantsbuild.slack.com/archives/C0D7T...
# development
c
cont. https://pantsbuild.slack.com/archives/C0D7TNJHL/p1669748426455479?thread_ts=1669746504.157219&amp;cid=C0D7TNJHL I don’t see any issues with supporting both, and you can choose which ever representation makes sense for you.. guess it’ll be a “product owner” question if we want this degree of flexibility.. ? EDIT: OR, make a switch from tuple based to dict based rule sets.
h
I generally prefer having one way to do things unless there's a good reason not to. Easier to document, maintain the code, etc
➕ 1
deprecation policy seems irrelevant since the feature is new
➕ 1
p
Yes. Only one way.
c
Yeah, now’s the time to decide.. 🙂
p
Using tuples works, the question becomes, do we want to maintain the python guarantee of dict ordering within BUILD files? They are parsed by the rust stuff, so if that’s not a safe guarantee, then we shouldn’t make it.
If we can use dicts, I think that would be a better UX. But, this works too.
b
They aren't parsed by Rust 😉 They are `eval`'d
(or `exec`'d, but same diff)
c
@proud-dentist-22844 does this work?:
Copy code
__dependencies_rules__(
  *{
    python_sources: ("src/foo", "src/bar", "!*"),
    (files, resources): ("res/**", "!*"),
  }.items()
)
that’s your dict syntax, converted to the tuple form expected now.. 😉
b
🤮
😉 1
p
lol. yeah, that’s doable.
b
@curved-television-6568 is there a reason to not prefer dict?
If anything, 
dict
prevents duplicate keys
c
I actually don’t care much either way at this point. I was pro-dict, but now I’m used to the tuple-way, so.. the thing is it is very clear that it is ordered, where as dict could be seens as any oder is fine, it’s a dict..
so if you’re not from a Python background, the tuple way will be less ambigous
b
As humans though, we do like order and I suspect our default schema for things is ordered
c
suspect our default schema for things is ordered
what’s this?
b
c
Ah, right.
as noted, I was fine with dict, but to @happy-kitchen-89482 (you’ll have to correct me if I’m wrong here) it wasn’t very clear what order the rules where going to be applied in with the dict representation.. We had a bit of back and forth, and I think I managed to confuse him quite a bit, but I missed to pick up on the notion of unordered dicts, he clearly stated here
FTR, I’m happy to do the switch back to dicts, if we get consensus for that.
h
I think dict sends the wrong message to the author. This isn’t, fundamentally, a mapping. It’s a list of rules, applied in order. At no point do we look up a value given a key.
p
lists/tuples however rely on positions in each rule. When someone first encounters this, it’s not clear:
Copy code
__dependencies_rules__(
  (
    ("python_*", "src/foo", "src/bar", "!*"),
    ("*", "res/**", "!*"),
  )
)
so, we have a series of globs - The first glob is for target types, and the others are path globs. But, nothing indicates that they are not all globbing files. A dict highlights that there is a relationship between one glob (or glob tuple) and the other globs.
@curved-television-6568 did you implement the more verbose form?
Copy code
__dependencies_rules__(
  (
    {"type": "python_*", "paths": ("src/foo", "src/bar", "!*")},
    {"type": "*", "paths": ("res/**", "!*")},
  )
)
If that’s valid, then I’ll probably use that all the time so it is easier to grok for newcomers to pants (which is basically everyone in the StackStorm community).
✅ 1
b
(I don't think looking up by key is a prereq for a mapping. I think saying "this maps to that" is the prereq. And it sounds like "this glob maps to these rules" fits)
c
@proud-dentist-22844 yes, but you’ve got the “paths” mixed up for the “rules”… it’s just the selectors that are in the dict, so:
Copy code
__dependencies_rules__(
  (
    ({"type": "python_*"}, "src/foo", "src/bar", "!*"),
    ({"type": "*"}, ("res/**", "!*")),  # you may group the rules though.. 
    ({"type": "file*", "path": "*.py"}, (...)),
  )
)
😵 1
p
yeah. That’s what I’ll use to highlight that it is different from the other globs.
c
internally, it uses the dict representation, the string syntax is just “syntactic sugar” : https://github.com/pantsbuild/pants/blob/c74dccdf589565d99b895303d01e8e7d68a5f45d/src/python/pants/backend/visibility/glob.py#L168
that’s why I think it’s not a bad thing to have some level of flexibility.. as there’s likely not going to exist a single representation that everyone is going to like 😉
😂 1
p
Does the syntatic sugar add much value? I’m afraid of using something obtuse. Though I suppose in a corporate env, “training” and “that’s how we do it here” is the way we do it.
b
I still haven't "consumed" what the visibility values all mean, but that looks very symbolic, hence the 😵 . I'm sure if I sat down and read the docs it'd "click" 🙂
🙃 1
c
Does the syntatic sugar add much value? I’m afraid of using something obtuse.
Though I suppose in a corporate env, “training” and “that’s how we do it here” is the way we do it.
I guess clarity over conciseness goes a long way in distributed project like st2, but in a closed corp env with a manageable sized group of ppl, training and later on-boarding for new hires will be required regardless and then the more concise syntax is more readable (in my eyes, guess that’s highly individual)
“real world” example, WIP trying this new feature out….
Copy code
# As a common rule, apps may only depend on current subtree, own models, templates, common and 3rd party libs.
APP_RULES = (
    "./**",
    "/models/**",
    "/../common/**",
    "/../djcommon/**",
    "/../templates/**",
    "//3rdparty/python*",
    "!*",
)


__dependencies_rules__(
    (
        # Users admin may depend on anything.
        "[/admin.py]", "*",
    ),
    (
        # Users permissions is an exception to the rules, as it may depend on deal models.
        "[/permissions.py]",
        "/../deals/models/**",
        APP_RULES,
    ),
    (
        # Viewsets and urls may depend on anything in the current app
        (
            "[/viewsets/**]",
            "[/urls/**]",
        ),
        "/**",
        APP_RULES,
    ),
    (
        # Libs, serializers and tasks may depend on anything in the current app except viewsets and urls
        (
            "[/libs/**]",
            "[/serializers/**]",
            "[/tasks/**]",
        ),
        "!/viewsets/**",
        "!/urls/**",
        "/**",
        APP_RULES,
    ),
    (
        # Everything else must follow the common app rules
        "*",
        APP_RULES,
    ),
)
looking at this, I actually think the dict syntax would make this worse, by being more nested than this flat list..
and having paths in 
[]
for the selectors, make them stand out from the rules..
b
Can you post the equivalent dict for comparison?
👍 1
(Also, woof the linecount on that bad boy 😛)
c
Copy code
# As a common rule, apps may only depend on current subtree, own models, templates, common and 3rd party libs.
APP_RULES = (
    "./**",
    "/models/**",
    "/../common/**",
    "/../djcommon/**",
    "/../templates/**",
    "//3rdparty/python*",
    "!*",
)


__dependencies_rules__(
    (
        # Users admin may depend on anything.
        {"path": "/admin.py"}, "*",
    ),
    (
        # Users permissions is an exception to the rules, as it may depend on deal models.
        {"path": "/permissions.py"},
        "/../deals/models/**",
        APP_RULES,
    ),
    (
        # Viewsets and urls may depend on anything in the current app
        (
            {"path": "/viewsets/**"},
            {"path": "/urls/**"},
        ),
        "/**",
        APP_RULES,
    ),
    (
        # Libs, serializers and tasks may depend on anything in the current app except viewsets and urls
        (
            {"path": "/libs/**"},
            {"path": "/serializers/**"},
            {"path": "/tasks/**"},
        ),
        "!/viewsets/**",
        "!/urls/**",
        "/**",
        APP_RULES,
    ),
    (
        # Everything else must follow the common app rules
        {},
        APP_RULES,
    ),
)
Oh, or did you mean dict if we swapped the syntax? I can do that too
b
yeah that 😛
c
Copy code
# As a common rule, apps may only depend on current subtree, own models, templates, common and 3rd party libs.
APP_RULES = (
    "./**",
    "/models/**",
    "/../common/**",
    "/../djcommon/**",
    "/../templates/**",
    "//3rdparty/python*",
    "!*",
)


__dependencies_rules__({
    # Users admin may depend on anything.
    "[/admin.py]": "*",

    # Users permissions is an exception to the rules, as it may depend on deal models.
    "[/permissions.py]": (
        "/../deals/models/**",
        APP_RULES,
    ),

    # Viewsets and urls may depend on anything in the current app
    (
        "[/viewsets/**]",
        "[/urls/**]",
    ): (
        "/**",
        APP_RULES,
    ),

    # Libs, serializers and tasks may depend on anything in the current app except viewsets and urls
    (
        "[/libs/**]",
        "[/serializers/**]",
        "[/tasks/**]",
    ): (
        "!/viewsets/**",
        "!/urls/**",
        "/**",
        APP_RULES,
    ),

    # Everything else must follow the common app rules
    "*": APP_RULES,
})
OK, that’s actually not so bad either.. 😉
BUT, it doesn’t support the dict based selectors, as they’re now dict keys
so will be less flexible going forward, if there’d be some feature we’d like to support that doesn’t fit the string syntax for the selectors
b
Not suggesting it, just offering. You could use types for this? 
PathRule(...)
vs 
TypeRule(...)
c
yeah… could 🙂
b
Then there wouldn't be an invented syntax and it'd be very explicit whats path v type.
c
using BUILD file macro, you can get this syntax now:
Copy code
# As a common rule, apps may only depend on current subtree, own models, templates, common and 3rd party libs.
APP_RULES = (
    "./**",
    "/models/**",
    "/../common/**",
    "/../djcommon/**",
    "/../templates/**",
    "//3rdparty/python*",
    "!*",
)


__dependencies_rules__(
    RuleSet(
        # Users admin may depend on anything.
        select_for=("[/admin.py]",),
        rules=("*",),
    ),

    RuleSet(
        # Users permissions is an exception to the rules, as it may depend on deal models.
        select_for=("[/permissions.py]",),
        rules=(
            "/../deals/models/**",
            APP_RULES,
        ),
    ),

    RuleSet(
        # Viewsets and urls may depend on anything in the current app
        select_for=(
            "[/viewsets/**]",
            "[/urls/**]",
        ),
        rules=(
            "/**",
            APP_RULES,
        )
    ),

    RuleSet(
        # Libs, serializers and tasks may depend on anything in the current app except viewsets and urls
        select_for=(
            "[/libs/**]",
            "[/serializers/**]",
            "[/tasks/**]",
        ),
        rules=(
            "!/viewsets/**",
            "!/urls/**",
            "/**",
            APP_RULES,
        )
    ),

    RuleSet(
        # Everything else must follow the common app rules
        select_for=("*",),
        rules=APP_RULES,
    ),
)
Copy code
def RuleSet(select_for, rules):
    return (select_for, rules)
@proud-dentist-22844 ☝️
and going in that direction, adding methods for declarative saying what is 
Path
and 
TargetType
etc…
I think this as a low level construct is well, and can be dressed with macros to reach any desired level of explicit verbosity
b
(I think in an ideal world, macros don't exist. To me they are a band-aid) I'd hope that the default syntax is also the more legible and expressive 🙂
c
well, it is to me 😛
but some prefers dicts, other key-value pairs, and me strings… there’s no single perfect world among us 😄
b
Well techincally-speaking there is a real difference between key/value pairs and a dict: duplicate key behavior
c
I’m even open to supporting both dicts and tuples, but then there’s purists that says there shall be one way to rule them all 🤣 (and the point has merit)
duplicate key behavior
if it comes down to that being a deciding factor, we could add a check that there should not be two identical selectors across all rule sets…
just saying 😉
p
There might be duplicates when extending parent role sets. That would basically mean the child rule would match first.
Yeah. Just leave it as is, and I'll use the 
({...}, ...)
format for rules.
Maybe we'll end up with a lint/fmt util that enforces a certain rule format in BUILD files. 🤔
b
I'll maintain this entire thing seems very DSL-shoved-into-a-round-hole to me
p
Yeah - but we also want to minimize the top-level BUILD symbol pollution. So, using an object is problematic. BUILD files are already essentially a DSL (that happens to use python syntax).
c
I’ll maintain this entire thing seems very DSL-shoved-into-a-round-hole to me
That’s the thing, it’s not a DSL, but pure Python… which gives me the idea that perhaps it ought to be a DSL, and as such live in a dedicated file outside of BUILD files… 😄
b
Here's what I had with the objects
Copy code
__dependencies_rules__({
    # Users admin may depend on anything.
    PathRule("/admin.py]): "*",

    # Users permissions is an exception to the rules, as it may depend on deal models.
    PathRule("/permissions.py"): (
        "/../deals/models/**",
        APP_RULES,
    ),

    # Viewsets and urls may depend on anything in the current app
    (
        PathRule"/viewsets/**"),
        PathRule("/urls/**"),
    ): (
        "/**",
        APP_RULES,
    ),

    # Libs, serializers and tasks may depend on anything in the current app except viewsets and urls
    (
        PathRule("/libs/**"),
        PathRule("/serializers/**"),
        PathRule("/tasks/**"),
    ): (
        "!/viewsets/**",
        "!/urls/**",
        "/**",
        APP_RULES,
    ),

    # Everything else must follow the common app rules
    "*": APP_RULES,

    # (Made up)
    TargetRule(python_source): ...
})
c
like a 
.gitignore
file, but with a different name (and syntax)
b
(You can have a DSL embedded in another language) 
SQLAlchemy
is DSL-esque
h
I still think it’s not a mapping. We have to iterate over it, and in order. The “keys” are just globs, not paths, or any notion of a key that the user holds. It is a tuple, not a map.
What is the advantage of treating it as a dict?
c
But we’re not introducing any specific language.. just syntax for applying plain python
My feeling is this comes down to how you map this problem mentally… looking at it from different viewpoints will give you different results for what makes most sense, I guess, well that’s my take any way 🙂
b
Perhaps, at the very least, you could make it an iterable of objects? Then you can introduce names (like you did with your macros example)
c
well, if your objects are iterable, that works already today, so we don’t need to change anything for that.
b
As it stands, there's a lot of symbols and not a lot of telling me what those symbols mean. And I think that's my issue
Without the comments, I absolutely couldn't grok the code snippet
c
can you give an example with objects where it’s clear, without comments?
this 
PathRule"/viewsets/**"),
doesn’t, to me say what it does. It’s a PathRule, but what does that mean?
b
It's at least more than a string. 
[somepath]
tells me even less
c
not buying ☝️
p
Conceptually, a rule is a conditional. I wonder if there's a way to lean into python's conditional syntax. Right now it is effectively 
rule[1:] if rule[0]
👍 1
c
I’m feeling a shift towards declaring the rules closer to the targets, as a ….. 
visibility
field 😮
👎 1
p
Uhm. Sorry for this can of worms... Just leave it as is so we can get it ASAP 😟
c
no worries Jacob 🙂
b
Just leave it as is so we can get it ASAP
That thinking makes me uncomfortable
c
I’m in favour of exploring this to an end is reached
b
At the very least, can we mark it experimental, so we can work on this longer than a release if needed but @proud-dentist-22844 gets the first draft?
c
it’s experimental already,
b
I'd say bring it up in the next meetup, but Dec's got canned
Is there a tracking issue?
c
and there’s a stbilizing ticket for it, mandating it be part of at least 2 stable releases before graduating (open for adjustments on graduation criterias)
b
c
that’s the one
b
OK I feel less bad. I'll start a ticket on UX
👍 1
Thanks for sticking it out. I care deeply about the UX of these flagship features
❤️ 1
I want dummies like me to be able to look at it and not scratch my head like a monkey staring at a chess board
c
🐒♟️
b
(which makes complete sense if you've played chess, or even chedckers. But monkey brain just says "hey you forgot to color in the other squares dummy")
c
well, I’m not sure if there’s a syntax that will describe this adequately, have you read the draft doc PR for this? https://github.com/pantsbuild/pants/pull/17632
Given the amount of docs, and it’s still not clear what all this does in my experience
p
The one thing that is not "experimental" are the names for 
__dependency_rules__
and 
__dependents_rules__
The implementation is in an experimental plugin.
c
true, the names themselves are not experimental 😄
but everything that goes into them are 😛
we have @proud-dentist-22844 to thank for that shift.. 🙂
what if the selectors where not it’s “own thing” as much, but more like we have a bunch of rules, and within those rules, there may be markers that say if we should skip the remaining rules or not, depending on what it is we’re working with. Example:
Copy code
__dependencies_rules__(
  "src/foo/bar/*",  # allow src/foo/bar/* for everything
  (  # start rule group, only for files and resources
      [resources, files],
      "static/**/*.res",
      "static/**/*.txt",
  ),
  (
      "tests/common/**",  # applies for all targets/paths
      [python_tests],  # any non-python_tests will exit this rule group here
      "tests/python/**",  # only python_tests will have this rule
  ),
  "!*",  # ALL will have this rule
)
I think this will allow reducing duplication in rules, by sharing common “rule sets”.
b
(another thought here what if we had N dependency rule targets instead of one encompassing one)
c
but rather than ☝️ we could allow it by accumulating the rules…
but, I’m not sure, think it may make sense to keep the rules together.. if you spread them out it can become harder to reason about and easier to miss if a certain rule applies that is somewhere else and you just didn’t see it..
b
Yeah it's also consistent with (fewer, plural target)
👍
c
I like the above, as it’s more a single flow, from top to bottom, with branching like if/then/else blocks sort of…
and it’s easy to terminate early, skipping the rest, with a catch all rule.
it also gets rid of the 
("*", …)
to mean, apply the 
…
rules to all targets
the only downside is, we’d need a way to distinguish the intermittent conditions from regular rule globs (used lists in the example above)
but perhaps it’s an OK restriction to impose, that all groups etc be tuples, and that conditions (selectors) be in lists.
Open in Slack
PreviousNext
Powered by