Refactor configuration and results information #111
Conversation
… 'dump' CLI options. Moved functions out of krun.util. Removed some complex logic from the scheduler.
…t for EntryPoint.__eq__.
…te class to simplify the '-dump' code. The scheduler now initialises VM definition objects.
snim2
force-pushed
the
refactor-config-results
branch
from
4337f2e
to
b1e154d
Compare
| @@ -101,109 +103,90 @@ def sanity_check_user_change(platform): | |||
| def create_arg_parser(): | |||
| """Create a parser to process command-line options. | |||
| """ | |||
| parser = argparse.ArgumentParser(description='Benchmark, running many fresh processes.') | |||
There was a problem hiding this comment.
All of the changes to this block are quote substitutions? Which rules are you using to decide which to use?
There was a problem hiding this comment.
Ah i see, you store a string of which dump.
There was a problem hiding this comment.
When I wrote the argument parsing code I used single quotes throughout, then I looked a bit further and realised that you had used double-quotes throughout. I thought I'd already changed all these to be consistent with your code, but apparently not.
Were you following some more complicated convention?
There was a problem hiding this comment.
I don't currently have any rule in place. I actually had to search to see what the convention is.
There's no difference between ' and " in python, but it seems people use ' to express the notion of a string which is constant and " for format strings, which are conceptually expanded.
E.g.:
'hello'"hello %s" % name
We could adopt such a notion if you like(?).
There was a problem hiding this comment.
Well, up to you, but maybe we should change code as we come to it rather than a big bang. Let me know if you want me to change the strings in this PR.
There was a problem hiding this comment.
Let's please not use arbitrary rules for "" and '' -- we'll never remember it, no-one will know why we did it, and so usage will quickly drift. Please just pick one and use it consistently in a file.
There was a problem hiding this comment.
Not sure what you mean Laurie. Do you mean use only " or ' in any given file?
There was a problem hiding this comment.
Yes. [And preferably be consistent in a project; but at least be consistent in a file.]
There was a problem hiding this comment.
Right. We may find that we can't be this black and white, for example when nesting quotes. But we can try if you really feel strongly.
Not sure I want to impose any rules TBH.
|
OK, so this looks good to me and it seems to work on my laptop. Do you want to do any squashing? |
snim2
force-pushed
the
refactor-config-results
branch
from
efb1f6e
to
e89e9a9
Compare
|
OK, I have squashed. There are only two small things I am not entirely happy with. One is that comparing two audits ignores the packages. This needs to be fixed as part of #103 The other thing is that the scheduler now sets the platform of the VM objects. Should anyone use Krun as a library, this is still rather opaque, but I couldn't think of a better place to put that code. |
|
I don't think these should block your merge. Overall this PR improves the quality of the code and those nits can be addressed later. Agree? |
|
Agree. I'll add the second point as a new issue. |
|
I really don't think we should bother with package checks and the like. It's too much effort for no real gain. Let's please drop it. |
|
OK, let's go. |
Refactor configuration and results information
This PR:
krun.audit.Audit,krun.config.Config,krun.results.Resultskrun.results.Resultsobjects from disk.krun.util.audits_same_platform, sincekrun.audit.Auditobjects can now be compared directly with==.unicode(Audit(...))is correctly formatted for--dump-audit.ExecutionSchedulerobjects now initialise VM definition objects, simplifying the logic inkrun.pyand making it less likely that clients of the library will forget to correctly initialise the objects.krun.results.Resultsobjects can serialise themselves on disk,krun.config.Configobjects can read from config files on disk or from a string.Fixes #75