WIP, NEED IDEAS: Move excludes logic into cmd_line_spec_parser so it can filter out broken build targets
Review Request #930 - Created Aug. 21, 2014 and discarded
|benjyw, jsirois, patricklaw|
Move excludes logic into cmd_line_spec_parser so it can filter out broken build targets. *** This is a work in progress, I don't want to commit things like this because it is extremely ugly. *** I wanted to add integration test in testprojects/ in a previous patch that had an intentionally broken BUILD file, but I couldn't because pants threw an exception early trying to parse BUILD files. pants goal test testprojects:: will fail because one of the targets has a bogus dependency. What I wanted to be able to do to get around this is: pants goal test testprojects:: --exclude-target-regexp=.*missing-build-file.* That did not work because the bad targets are first injected into the graph, then filtered out later. The patch in this state fixes that issue by moving the --exclude-target-regexp code into cmd_line_spec_parser.py so that the build graph never sees them, but there are 2 problems: 1) The options aren't parsed yet when CmdLineSpecParser is instantiated. I need the --exclude-target-regexp values. I included an ugly hack to get around that. 2) The logger also isn't initialized yet because of problem #1) I had a hack where I deferred printing the debug messages until run(), but surely there are other places where we want to log early. I could replicate the ugly hack and examine the command line early in pants execution to turn on the logger... but I was hoping someone would have a better idea.
|Don't use mutable literals for default args, it's dangerous. Instead, default to `None` and then reset the variable immediately with ...||Patrick Lawson|
Afaict there is no way much better than what you do for hack #1. For #2, I'd prefer use #1 for the debug flag - aka peek at the log level and using plain old print to stderr if debug. The #2 bit could be wrapped up in a helper of some sort for the very few components that need pre-boot logging. Benjy's head has been in this space so I'm hoping he has news of magiv from Argparse land or from his own head.
This really feels like a stretch. Broken targets are not something that you'd ever want in your repository, unless you're a pants developer. As discussed on the test_projects review, "negative" tests should almost certainly just be defined as unit tests (with whatever additions we need to the unit test framework to allow for that.)
Don't use mutable literals for default args, it's dangerous. Instead, default to `None` and then reset the variable immediately with `target_excludes_opts = target_excludes_opts or ` (I know this is a strawman CR, but this is one of those bad ones that's easy to forget about if you do end up moving forward with the patch)