thanks

i usually grep first and ask questions later

but last night was out of it oh well

I N C R E D I B L E

if i had just grepped

./pants list src::

we would have been done here

thanks, the other CI error is a real one but i'm gonna knock that out asap

@average-vr-56795 when i saved

release.sh

(thanks to stu's comment just above), my editor converted some tabs in indentation of the body of a case statement starting on line 555 into spaces. the modified region which was not part of the intended edits to the file (the region where tabs were converted to space) is 10 lines and is easy to audit -- would that be acceptable to put in this pr (#5747), or is it pretty much always better to leave that sort of thing out (which is very easy for me to do, i can add a dir-local variable to stop all whitespace edits automatically)?

random thought: it almost definitely wouldn't be impossible to add a few lines to pre-commit that find hunks of pure-whitespace changes and reverts them (would probably add a confirmation prompt to that if i did though)

that might actually be useful and extensible to not just whitespace

i expanded the engine readme a bit, let me know if the "Datatypes in Depth" section is complete nonsense -- i think it's great but it might still be nonsense https://github.com/pantsbuild/pants/pull/5759

@aloof-angle-91616 If we want to kill tabs, I think my preference would be to create a separate PR which adds a

grep -rIl "\t" | grep -v '\.go$'

check to build-support/bin/pre-commit.sh, gets rid of all existing tabs, and then being happy that this will never be a problem again 🙂

yep i already reverted that in this pr

Thanks!

CI is green and this is reviewed: https://github.com/pantsbuild/pants/pull/5747

i rebased which is why there are so many commits after nora's comment

are you looking for someone to merge it?

@fancy-queen-20734 yes! ideally

unblocks another pr i want to take a look at over the weekend

added a warning when

sources

and

bundles

globs don't match any of the target's files, will add an exception tomorrow but just wanted to get it out now https://github.com/pantsbuild/pants/pull/5769

i improved the above by adding the incredible new feature of passing travis

@aloof-angle-91616 When I added the “error on imports in BUILD files” feature, I ended up with a tri-state flag: allow/warn/error, which I suspect would sit nicely with this feature too

In particular, people steered me away from re-parsing the global options, which it looks like your PR probably does?

I haven’t looked too closely, as I definitely don’t have much context on the code to suggest improvements, but the idea looks great to me 🙂

@average-vr-56795 the tri-state flag sounds great and i can totally implement that. can you explain what you mean by "re-parsing the global options"?

oh, you're totally right. i literally was using options passed in and then didn't know if that was right

i'll make that fix (i'm 99% sure it's why i'm seeing the error in unit test ci right now)

i also noticed that COMPLETELY by accident the method of overriding the

__new__()

constructor for `datatype`s literally has the exact same mechanics and syntax as in R S4 classes, down to the variadic args: R uses

...

for positional and keyword args (you can split them up easily too) but python introduces the separation, but otherwise this is literally the exact same code as an S4 constructor override in R.

the line in question being

def __new__(cls, *args, **kwargs):
    this_object = super(GlobMatchErrorBehavior, cls).__new__(cls, *args, **kwargs)
    # do stuff with the constructed object...

in R the equivalent would be roughly (a snippet i pulled from somewhere):

setMethod('initialize', 'Type', function(.Object, name, ...) {
    .Object <- callNextMethod(.Object, ...)
    .Object@name <- validateTypeNameString(name)
    .Object
})

'initialize'

is the method to override for the constructor (objects are created with

new(...)

, however)