I think it's a nice improvement. Passing the options around via the argparse results (as I do in many programs) makes it easier to unit test, but having configuration variables all in a module makes it really simple to find and use everywhere without having them as globals.
Thanks for cleaning that up, Zachary! -Todd On Tue, Dec 8, 2015 at 11:31 AM, Greg Clayton via lldb-dev < lldb-dev@lists.llvm.org> wrote: > Sounds good, looks good then. > > > On Dec 8, 2015, at 11:09 AM, Zachary Turner <ztur...@google.com> wrote: > > > > One advantage of this approach is that it makes the options available to > the entire test suite. Even if we have no transferring going on, and we > get argparse to return us a perfectly organized structure with everything > in the right format, in order to make all the options accessible to the > rest of the test suite, we still need to stick it in a global module > somewhere. And then you would write > `configuration.options.test_categories`, whereas with this approach we just > write `configuration.test_categories`. It's a minor point, but I like the > shorter member access personally. > > > > On Tue, Dec 8, 2015 at 11:07 AM Zachary Turner <ztur...@google.com> > wrote: > > There's no way to avoid doing a transfer out of the options dictionary > at some level, because it's not a straight transfer. There's a ton of > post-processing that gets done on the options dictionary in order to > convert the raw options into a useful format. > > > > That might be solvable with more advanced use of argparse. This > approach does get rid of one level of option transfer though. Because we > would transfer > > 1. From the class returned by argparse into the global > > 2. From the global into the lldb module > > > > Now we only transfer from the argparse class into the `configuration` > module, and everything else just uses that. > > > > > > On Tue, Dec 8, 2015 at 10:52 AM Greg Clayton <gclay...@apple.com> wrote: > > Do we not want to have an "options" global variable in this module that > contains everything instead of having separate global variables in this > file? The idea would be that you could assign directly when parsing > arguments: > > > > (configuration.options, args) = parser.parse_args(sys.argv[1:]) > > > > Its OK if we don't do this, but this is what I was originally thinking. > Then we don't need to do any transfer out of the options dictionary that is > returned by the option parser. The drawback with this approach is the > "configuration.options" would probably need to be initialized in case > someone tries to access the "configuration.options" without first parsing > arguments. So in that respect the global approach is nicer. > > > > Greg > > > > > On Dec 8, 2015, at 10:45 AM, Zachary Turner <ztur...@google.com> > wrote: > > > > > > Hi Greg, > > > > > > Take a look at dotest.py next time you get some free time and let me > know what you think. There should be no more globals. Everything that > used to be a global is now stored in its own module `configuration.py`, and > everything in `configuration.py` can be referenced from everywhere in the > entire test suite. > > > > > > On Fri, Nov 20, 2015 at 10:34 AM Greg Clayton <gclay...@apple.com> > wrote: > > > Zach, I would also like to get rid of all global variables in the > process of this change. The history goes like this: a long time ago someone > wrote the initial dotest.py and parsed the options manually and stored > results in global variables. Later, someone converted the options over to > use a python library to parse the options, but we mostly copied the options > from the options dictionary over into the globals and still use the globals > all over the code. It would be great if we had at most one global variable > that is something like "g_options" and anyone that was using any global > variables will switch over to use the "g_options.XXXX" instead. Then we > don't have to make copies and we can let the g_options contain all settings > that are required. > > > > > > > On Nov 18, 2015, at 2:32 PM, Zachary Turner via lldb-dev < > lldb-dev@lists.llvm.org> wrote: > > > > > > > > I would like to do a complete audit of dotest's command line > options, find out who's using what, and then potentially delete anything > that isn't being used. There's a mess of command line options in use, to > the point that it's often hard to find free letters to use for new options. > > > > > > > > I created this spreadsheet with a complete list of command line > options, their descriptions, and a place for people to enter what options > they're using or do not want to be deleted. > > > > > > > > > https://docs.google.com/spreadsheets/d/1wkxAY7l0_cJOHhhsSlh3aKKlQShlX1D7X1Dn8kpqxy4/edit?usp=sharing > > > > > > > > If someone has already written YES in the box that indicates they > need the option, please don't overwrite it. If you write YES in a box, > please provide at least a small rationale for why this option is useful to > you. Feel free to add additional rationale if someone has already added > some rationale. > > > > > > > > I'm going to have a couple days in mid-December and do this cleanup, > so I'd like to get a solid picture of what options are not needed before > then. After people have had some time to look over this, I'll go through > the results and decide what to do with each one, and then send out another > email with a proposed action column for each command line option. > > > > > > > > Please do take the time to have a look at this, because any option > that doesn't have a YES in it after a couple of weeks I'm going to assume > is a candidate for deletion. > > > > > > > > > > > > _______________________________________________ > > > > lldb-dev mailing list > > > > lldb-dev@lists.llvm.org > > > > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev > > > > > > > _______________________________________________ > lldb-dev mailing list > lldb-dev@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev > -- -Todd
_______________________________________________ lldb-dev mailing list lldb-dev@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
