refactor(sql): Dispatch for sql. Move resolver source to scope

[dustmop]

Use the Dispatch pattern for sql methods.

Instead of source being a parameter that is passed down the callstack, until code calls LoadDataset or ParseAndResolveRef, attach source to the scope. This will let us remove the source from the interface, since the callsite of LoadDataset doesn't care about source, the concrete implementation of the Loader does. Clients of lib can configure the source just-in-time by using WithSource on the Instance.

Also, discovered that the --offline flag for sql was broken, so resolution would never be local-only. The cause was ParseResolveFunc always using the defaultResolver. This type of cleanup should make these sorts of bugs much harder to cause.

Finally, remove the ResolverForMode function from scope. Only lib.Save was using it, but in actually it only needs a localResolver, so it uses that instead now.

[b5]

Big fan of these changes. I'd like your input on a WithConfig method, and we need to un-skip an integration test

[b5]

I think this is fine for the short term, but I'd like to see this generalized to be about configuration overrides. We can't do that now because we don't expose the resolver source in configuration.

I think InstanceConfigWrap could be replaced by a WithConfig method on Instance that returns a new Instance pointer with an updated internal configuration. inst.WithSource would either be deprecated in favor of inst.WithConfig, or could become a wrapper:

func (inst *Instance) WithResolveSource(source string) *Instance {
  cfg := inst.cfg.Copy()
  cfg.ResolverSource = source
  return inst.WithConfig(cfg)
}

If you're ok with this approach I think the right path forward would be to file an issue stating we need a ResolverSource field on Config (name of the field is totally debatable), then we merge this and close the issue with a follow up PR / refactoring.

[dustmop]

I'm fine with changing InstanceConfigWrap, I don't like it much either. It's there because Instance is a bit heavy weight, and copying it entirely is expensive. I could see adding the source to config as one option. Another would be to have Instance's owned subsystems (like the bus, p2p node, etc) on some inner struct, such that it would be cheap to construct another Instance that points to the same inner struct, while other state can be mutated freely.

For the field name I'm fine with either ResolverSource or even just Source.

My only hesitation on adding source to config is that it feels more like a per-command kind of thing, similar to --no-prompt and --no-color. I've been mentally classifying it as somewhere between config and a normal parameter, rather like a "mode" or something. As a thought experiment: if Qri Desktop gets some UI to change the resolver source, like a slider or drop-down or something, would changing that value modify a config file on disk, or would it just change some value in memory? I'm picturing the latter.

[b5]

Yeah in my head WithConfig would construct shallow-copy *Instance . Specifically a method like WithConfig should not construct new copies of subsystems. With access to subsystems governed by scope I think that's a safe approach to take.

As for the config change, I vey much see your point on adding a Source field to config, and think your mental model of it being an "Option" according to the parlance laid out here:

qri/lib/lib.go

Lines 63 to 70 in 9f36fc5

	// InstanceOptions provides details to NewInstance.
	// New will alter InstanceOptions by applying
	// any provided Option functions
	// to distinguish "Options" from "Config":
	// * Options contains state that can only be determined at runtime
	// * Config consists only of static values stored in a configuration file
	// Options may override config in specific cases to avoid undefined state
	type InstanceOptions struct {

To capture setting both "Config" and "Options", we could flip to a WithOptions method instead of a WithConfig, and use the OptConfig function to override configurations.

But now we're actively modeling a different issue, so I'm good to merge this as-is & think we should move this discussion to an issue.

[b5]

This added source param adds to my belief that source needs to be in configuration (and all configuration should be overridable)

[b5]

Not a massive fan of the OutputFormat -> Format switch. To me the test in lib/integration_test.go now reads kinda like the format of the query is JSON.

That said, this might be in the name of aligning field names. Care to clarify why you made the change?

[dustmop]

Yes, done to align the field name with other commands. For example list and diff both use Format to talk out about their output format, and all three of these fall into the "aggregation" category of commands. It also matches the command-line flag --format already in use for the command.

I sort of see what you mean about the ambiguity of input/output format, but the context makes it clearer.

[b5]

This is an important test. Can't skip. let's instead figure out what's going on before merging

[dustmop]

Will take a look and come up with a fix.

[dustmop]

The spike at c5c65b8 was super helpful. Pushed another commit that fixes the test.

What was happening is that the implementation of dataset.pull overrides the source with "network" if a blank source is passed in. However, this change is moving to a world where the scope locks-in the source, and implementations should not be allowed to change it.

I see 3 options going forward:

1. Require users of pull to always do this:

instance.WithSource("network").Pull(...)

This is the change I went with because it seems easiest. Now cmd calls WithSource('network'). In the future we can add a method to scope to get (but not set) the source, and pull can fail if it's not "network".

2. Add a method to scope to override the source. pull can call this to set the source. I'm not a fan of this option because it goes against what this PR is trying to do, but if it's only needed in a few places it's not that bad.

3. Add a field to Attributes to set the default source for commands. This will usually be empty, but can be "network" for pull. I don't think we should do this until we find more places that need it.

[b5]

Very much agree this is the right approach for now. This PR makes the need for the override explicit, which I'm a huge fan of. Option 3 (adding a field to a method set's returned Attributes) seems like a reasonable design choice, but I'm with you that it's too soon to make such a change

[b5]

LGTM!