Code review comment for lp:~axwalk/juju-core/lp1235717-refactor-simplestreams-writemetadata
Revision history for this message
| Andrew Wilkins (axwalk) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
0e1f616
(Get the code!)
https:/
File cmd/plugins/
https:/
cmd/plugins/
tools.MergeAndW
tools.ResolveFl
On 2013/10/09 12:08:57, rog wrote:
> I'd prefer to see explicit values for the resolve flag here - using it
as a
> boolean gets around the advantages of using the named constants, I
think.
Done.
https:/
File environs/
https:/
environs/
On 2013/10/09 12:08:57, rog wrote:
> s/metadata/size and hash/ ?
Done.
https:/
environs/
On 2013/10/09 12:08:57, rog wrote:
> if md.Size != 0 {
> continue
> }
> to save an indent level?
Done.
https:/
environs/
oldMetadata []*ToolsMetadata) []*ToolsMetadata {
On 2013/10/09 12:08:57, rog wrote:
> I'm not sure that "new" and "old" are appropriate prefixes here - they
seem to
> imply that the arguments are treated differently, but isn't
MergeMetadata
> commutative?
> Personally I'd use something like m0 and m1, but go with your own
thoughts for
> naming.
It's not commutative, per the last sentence of the doc comment:
"If both entries have information, prefer the entry from newMetadata".
The idea behind that is that if both have size&hash, then the one from
"newMetadata" is considered newer. That's to cater for copying over the
top of existing tools (say, a dev build).
https:/
environs/
On 2013/10/09 12:08:57, rog wrote:
> I know we do this elsewhere, but I've come to dislike
> this idiom - it's an unsatisfactory halfway house between
> named constants and bools. There's still code that
> uses the bool semantics, which means that these names aren't
> really helping much because you have to know their values
> anyway.
> I'd prefer either to use int and iota or to stick with the bool
> and lose the named constants.
Consider a bool as (conceptually) a 1-bit unsigned int; then what's the
difference between that and int with constant named values? The only
difference I see is that the bool is range limited to [0, 1], which is a
very good thing.
I'd be happy to drop the named type. Would that be a reasonable
alternative? i.e.:
const (
DontResolve = false
Resolve = true
)
func MergeAndWriteMe
coretools.List, resolve bool) error {
...
}
then people can use bools if they want, without much remorse, or they
can use the named constants as documentation.
https:/
environs/
generated from the given tools list.
On 2013/10/09 12:08:57, rog wrote:
> It's not clear what targetTools is here. It sounds like it's
> a list of tools in the target storage, but it doesn't actually
> seem like that's the case.
It is a list of tools in the target storage (it doesn't really make
sense to write metadata for anything else, as tools paths are relative
to the metadata).
I'm going to drop "target" in all cases, because there is no knowledge
of "source" in this function.
https:/
environs/
metadata is resolved by fetching the tools
On 2013/10/09 12:08:57, rog wrote:
> // If resolve is Resolve
I've dropped the named type and made it a bool, so I'll leave this
alone.
https:/
environs/
storage.Storage, targetTools coretools.List, resolve ResolveFlag) error
{
On 2013/10/09 12:08:57, rog wrote:
> s/stor/target/ ?
> (given that we refer to "the target storage" above)
I went the other way round: s/ target//.
https:/
File environs/
https:/
environs/
TestMergeMetadata(c *gc.C) {
On 2013/10/09 12:08:57, rog wrote:
> I think this might read better as a table-driven test,
> with each item in the table containing the inputs and the
> expected output.
> Then it's easy to check commutativity in a uniform
> way, for example.
Done. (Not commutative as per my other comment, so no change in
coverage.)
https:/