Merge environment and compare environments

[karthiknadig]

No description provided.

[codecov-commenter]

Codecov Report

Merging #14026 into main will decrease coverage by 0.06%.
The diff coverage is 44.24%.

@@            Coverage Diff             @@
##             main   #14026      +/-   ##
==========================================
- Coverage   59.92%   59.85%   -0.07%     
==========================================
  Files         703      704       +1     
  Lines       38933    39015      +82     
  Branches     5626     5644      +18     
==========================================
+ Hits        23331    23353      +22     
- Misses      14395    14439      +44     
- Partials     1207     1223      +16     

Impacted Files	Coverage Δ
src/client/pythonEnvironments/base/info/env.ts	21.91% <21.91%> (ø)
src/client/pythonEnvironments/info/index.ts	40.29% <40.00%> (-1.24%)	⬇️
...ient/pythonEnvironments/base/info/pythonVersion.ts	66.66% <50.00%> (-4.77%)	⬇️
src/client/pythonEnvironments/base/info/index.ts	100.00% <100.00%> (+6.66%)	⬆️
...nts/base/locators/composite/environmentsReducer.ts	95.00% <100.00%> (+0.06%)	⬆️
...ts/base/locators/composite/environmentsResolver.ts	95.58% <100.00%> (ø)
...pythonEnvironments/common/environmentIdentifier.ts	96.96% <100.00%> (+5.30%)	⬆️
src/client/common/utils/platform.ts	68.00% <0.00%> (-4.00%)	⬇️
src/client/logging/levels.ts	61.11% <0.00%> (ø)
src/client/logging/logger.ts	62.74% <0.00%> (ø)
... and 3 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 57fedbd...ecae324. Read the comment docs.

[karrtikr]

I recommend using lodash.cloneDeep instead of using the spread operator, other than that the heuristic LGTM.

[karrtikr]

Doc comment would be useful.

[karrtikr]

Can you move this to src\client\pythonEnvironments\base\info\pythonVersion.ts?

[karthiknadig]

Will do.

[ericsnowcurrently]

Overall this seems good. There are only a few things that I've noted which are related to correctness.

[ericsnowcurrently]

If you are checking release level then you should be checking release serial too.

[ericsnowcurrently]

What we really care about is picking a winner between two envs, with kind as our current heuristic. With a slight change this function can accommodate a bit more utility and flexibility:

Suggested change

	function pickEnvironmentKind(left: PythonEnvInfo, right: PythonEnvInfo): PythonEnvKind {
	function sortByPriority(left: PythonEnvInfo, right: PythonEnvInfo): [PythonEnvInfo, PythonEnvInfo] {

Note that this suggestion is inspired by a use case I had for putting the envs in priority order.
and

[ericsnowcurrently]

FWIW, it wouldn't be much of a stretch to accommodate a variadic input, which I could see being useful occasionally:

Suggested change

	function pickEnvironmentKind(left: PythonEnvInfo, right: PythonEnvInfo): PythonEnvKind {
	function sortByPriority<T extends PythonEnvInfo[]>(...envs: T): T {

(That's a new feature of Typescript 4.0.)

[ericsnowcurrently]

What about all the other parts of the not-preferred env that we should use to fill in gaps in the preferred one (e.g. distro, arch, searchLocation)?

[ericsnowcurrently]

As noted elsewhere, I'm planning on removing the id property. It turned out to not be useful.

Suggested change

	// tslint:disable-next-line: no-suspicious-comment
	// TODO: compute id for the merged environment
	merged.id = '';

[ericsnowcurrently]

I'm still unclear on what the heuristic approach buys us over a more straightforward merge.

    const merged = copyEnvInfo(preferred);
    merged.version = mergeVersions(preferred.version, other.version);
    merged.executable = mergeExecutable(preferred.executable, other.executable);
    if (preferred.arch === Architecture.Unknown) {
        merged.arch = other.arch;
    }
    ...

(Note that copyEnvInfo() is in PR #14051 and we would have to add mergeVersions().)

[karrtikr]

We shouldn't mix preferred.version and other.version, as they're likely discovered in chunks and so only valid as a unit. Heuristic allows us to choose one of them instead of merging.

[karthiknadig]

I discussed this with @karrtikr, we want to preserver executable and version as chunks. as they are always gathered as chunks. All other bits of info can be mix and matched.

[ericsnowcurrently]

What happens when both have filename, one has sysPrefix, and the other has ctime and mtime? I'd expect the merge to have all of those values.

As to merging the version, using a heuristic seems like more of an implementation detail, while the actual operation is mergeVersions().

[karthiknadig]

To me executable should be treated as a unit and so should version. From what I can tell sysPrefix does not belong in the executable object.

As for versions, let's say one version object has major and minor, and the other has minor, and micro. I would not trust the object that just has minor and micro without major. So we need to treat these as a whole (all or nothing).

[karthiknadig]

As for sysPrefix, I think it needs to be moved out of the executable, it does not belong there.

[ericsnowcurrently]

Prioritization (via pickEnvironmentKind()) and merging seem like two distinct operations. The former is specific to the "reducer" locator that Kartik is working on (the function would live there un-exported) and the latter is broadly applicable (the function would still be exported here). So this function would look like this:

Suggested change

	export function mergeEnvironments(left: PythonEnvInfo, right: PythonEnvInfo): PythonEnvInfo {
	const kind = pickEnvironmentKind(left, right);
	/**
	* Make a copy of the given env, merging the other into it where info is missing.
	*
	* @param env - the object to copy and fill in missing info
	* @param other - the object to use to fill in missing info
	*/
	export function mergeEnvironments(env: PythonEnvInfo, other: PythonEnvInfo): PythonEnvInfo {

and callers would use it like this:

    const [preferred, other] = sortByPriority(env1, env2);
    const merged = mergeEnvironments(preferred, other);

Just to be clear, the first arg to mergeEnvironments() is the one treated as authoritative, copied, and has any missing info filled in from "other".

[ericsnowcurrently]

Of course we could also apply variadic args if it would be useful.

Suggested change

	export function mergeEnvironments(left: PythonEnvInfo, right: PythonEnvInfo): PythonEnvInfo {
	const kind = pickEnvironmentKind(left, right);
	export function mergeEnvironments(env: PythonEnvInfo, others: ...PythonEnvInfo): PythonEnvInfo {

[karrtikr]

We cannot use the preferred environment using just kind of environment, so this won't work.
const [preferred, other] = sortByPriority(env1, env2);

Note the reducer also expects to choose between versions, executables etc.
env1 may contain the better version but env2 might contain the correct kind. So we cannot use the preferred environment. Only while merging them, can we start choosing. That would also mean that the priority logic won't be separate from the merge.

[ericsnowcurrently]

Maybe we should discuss this some more. I'm not following your argument against separating sortByPriority() from mergeEnvs(). I expect I wasn't clear enough about how that would work.

[ericsnowcurrently]

This should go in src/client/pythonEnvironments/base/info/env.ts with the other helpers that deal directly with PythonEnvInfo.

I recommend moving the rest of the file to the file where Kartik is putting the "reducer" locator: src/client/pythonEnvironments/base/locators/composite/environmentsReducer.ts. (Note that his PR has it in src/client/pythonEnvironments/collection/environmentsReducer.ts but only for the moment.) That way it will cause a merge conflict for the PR that doesn't get merged first, ensuring that we put these "reducer"-specific helpers in the right place.

[karrtikr]

LGTM otherwise

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
0.0% Duplication