https://pantsbuild.org/ logo
Powered by
c

curved-television-6568

11/29/2022, 7:08 PM
cont. https://pantsbuild.slack.com/archives/C0D7TNJHL/p1669748426455479?thread_ts=1669746504.157219&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

hundreds-father-404

11/29/2022, 7:09 PM
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

proud-dentist-22844

11/29/2022, 7:09 PM
Yes. Only one way.
c

curved-television-6568

11/29/2022, 7:10 PM
Yeah, now’s the time to decide.. 🙂
p

proud-dentist-22844

11/29/2022, 7:11 PM
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

bitter-ability-32190

11/29/2022, 7:13 PM
They aren't parsed by Rust 😉 They are `eval`'d
(or `exec`'d, but same diff)
c

curved-television-6568

11/29/2022, 7:13 PM
@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

bitter-ability-32190

11/29/2022, 7:13 PM
🤮
😉 1
p

proud-dentist-22844

11/29/2022, 7:14 PM
lol. yeah, that’s doable.
b

bitter-ability-32190

11/29/2022, 7:14 PM
@curved-television-6568 is there a reason to not prefer dict?
If anything, 
dict
prevents duplicate keys
c

curved-television-6568

11/29/2022, 7:16 PM
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

bitter-ability-32190

11/29/2022, 7:17 PM
As humans though, we do like order and I suspect our default schema for things is ordered
c

curved-television-6568

11/29/2022, 7:17 PM
suspect our default schema for things is ordered
what’s this?
b

bitter-ability-32190

11/29/2022, 7:18 PM
c

curved-television-6568

11/29/2022, 7:19 PM
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

happy-kitchen-89482

11/29/2022, 7:40 PM
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

proud-dentist-22844

11/29/2022, 7:50 PM
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

bitter-ability-32190

11/29/2022, 7:54 PM
(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

curved-television-6568

11/29/2022, 8:43 PM
@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

proud-dentist-22844

11/29/2022, 8:44 PM
yeah. That’s what I’ll use to highlight that it is different from the other globs.
c

curved-television-6568

11/29/2022, 8:45 PM
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

proud-dentist-22844

11/29/2022, 8:47 PM
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

bitter-ability-32190

11/29/2022, 8:48 PM
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

curved-television-6568

11/29/2022, 8:51 PM
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

bitter-ability-32190

11/29/2022, 8:57 PM
Can you post the equivalent dict for comparison?
👍 1
(Also, woof the linecount on that bad boy 😛)
c

curved-television-6568

11/29/2022, 8:59 PM
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

bitter-ability-32190

11/29/2022, 9:00 PM
yeah that 😛
c

curved-television-6568

11/29/2022, 9:02 PM
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

bitter-ability-32190

11/29/2022, 9:03 PM
Not suggesting it, just offering. You could use types for this? 
PathRule(...)
vs 
TypeRule(...)
c

curved-television-6568

11/29/2022, 9:03 PM
yeah… could 🙂
b

bitter-ability-32190

11/29/2022, 9:04 PM
Then there wouldn't be an invented syntax and it'd be very explicit whats path v type.
c

curved-television-6568

11/29/2022, 9:06 PM
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

bitter-ability-32190

11/29/2022, 9:11 PM
(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

curved-television-6568

11/29/2022, 9:11 PM
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

bitter-ability-32190

11/29/2022, 9:12 PM
Well techincally-speaking there is a real difference between key/value pairs and a dict: duplicate key behavior
c

curved-television-6568

11/29/2022, 9:12 PM
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

proud-dentist-22844

11/29/2022, 9:17 PM
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

bitter-ability-32190

11/29/2022, 9:20 PM
I'll maintain this entire thing seems very DSL-shoved-into-a-round-hole to me
p

proud-dentist-22844

11/29/2022, 9:24 PM
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

curved-television-6568

11/29/2022, 9:25 PM
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

bitter-ability-32190

11/29/2022, 9:26 PM
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

curved-television-6568

11/29/2022, 9:26 PM
like a 
.gitignore
file, but with a different name (and syntax)
b

bitter-ability-32190

11/29/2022, 9:26 PM
(You can have a DSL embedded in another language) 
SQLAlchemy
is DSL-esque
h

happy-kitchen-89482

11/29/2022, 9:27 PM
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

curved-television-6568

11/29/2022, 9:27 PM
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

bitter-ability-32190

11/29/2022, 9:29 PM
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

curved-television-6568

11/29/2022, 9:30 PM
well, if your objects are iterable, that works already today, so we don’t need to change anything for that.
b

bitter-ability-32190

11/29/2022, 9:30 PM
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

curved-television-6568

11/29/2022, 9:32 PM
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

bitter-ability-32190

11/29/2022, 9:32 PM
It's at least more than a string. 
[somepath]
tells me even less
c

curved-television-6568

11/29/2022, 9:33 PM
not buying ☝️
p

proud-dentist-22844

11/29/2022, 9:34 PM
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

curved-television-6568

11/29/2022, 9:35 PM
I’m feeling a shift towards declaring the rules closer to the targets, as a ….. 
visibility
field 😮
👎 1
p

proud-dentist-22844

11/29/2022, 9:36 PM
Uhm. Sorry for this can of worms... Just leave it as is so we can get it ASAP 😟
c

curved-television-6568

11/29/2022, 9:36 PM
no worries Jacob 🙂
b

bitter-ability-32190

11/29/2022, 9:36 PM
Just leave it as is so we can get it ASAP
That thinking makes me uncomfortable
c

curved-television-6568

11/29/2022, 9:37 PM
I’m in favour of exploring this to an end is reached
b

bitter-ability-32190

11/29/2022, 9:38 PM
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

curved-television-6568

11/29/2022, 9:38 PM
it’s experimental already,
b

bitter-ability-32190

11/29/2022, 9:38 PM
I'd say bring it up in the next meetup, but Dec's got canned
Is there a tracking issue?
c

curved-television-6568

11/29/2022, 9:38 PM
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

bitter-ability-32190

11/29/2022, 9:38 PM
c

curved-television-6568

11/29/2022, 9:39 PM
that’s the one
b

bitter-ability-32190

11/29/2022, 9:39 PM
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

curved-television-6568

11/29/2022, 9:40 PM
🐒♟️
b

bitter-ability-32190

11/29/2022, 9:40 PM
(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

curved-television-6568

11/29/2022, 9:42 PM
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

proud-dentist-22844

11/29/2022, 9:42 PM
The one thing that is not "experimental" are the names for 
__dependency_rules__
and 
__dependents_rules__
The implementation is in an experimental plugin.
c

curved-television-6568

11/29/2022, 9:42 PM
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

bitter-ability-32190

11/29/2022, 10:16 PM
(another thought here what if we had N dependency rule targets instead of one encompassing one)
c

curved-television-6568

11/29/2022, 10:16 PM
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

bitter-ability-32190

11/29/2022, 10:19 PM
Yeah it's also consistent with (fewer, plural target)
👍
c

curved-television-6568

11/29/2022, 10:20 PM
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.
Powered by