Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

show editing package version like editing 1.2.3 #379

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

show editing package version like editing 1.2.3 #379

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f48ad81
fixed read dependencies from workspace-state.json
aelam Jul 30, 2022
cfcdc65
clean unused import
aelam Jul 30, 2022
5b05cce
editing package will show pattern like `editing 1.2.3`
aelam Jul 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion src/SwiftPackage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,9 +117,20 @@ export interface WorkspaceState {
version: number;
}

/** revision + (branch || version)
* ref: https://github.com/apple/swift-package-manager/blob/e25a590dc455baa430f2ec97eacc30257c172be2/Sources/Workspace/CheckoutState.swift#L19:L23
*/
export interface CheckoutState {
revision: string;
branch: string | null;
version: string | null;
}

export interface WorkspaceStateDependency {
basedOn: WorkspaceStateDependency | null;
packageRef: { identity: string; kind: string; location: string; name: string };
state: { name: string; path?: string };
state: { name: string; path?: string; checkoutState?: CheckoutState };
subpath: string;
}

export interface PackagePlugin {
Expand Down
123 changes: 91 additions & 32 deletions src/ui/PackageDependencyProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,12 @@ import * as vscode from "vscode";
import * as fs from "fs/promises";
import * as path from "path";
import configuration from "../configuration";
import { getRepositoryName, buildDirectoryFromWorkspacePath } from "../utilities/utilities";
import { buildDirectoryFromWorkspacePath } from "../utilities/utilities";
import { WorkspaceContext } from "../WorkspaceContext";
import { FolderEvent } from "../WorkspaceContext";
import { FolderContext } from "../FolderContext";
import contextKeys from "../contextKeys";
import { WorkspaceState } from "../SwiftPackage";
import { WorkspaceState, WorkspaceStateDependency } from "../SwiftPackage";

/**
* References:
Expand Down Expand Up @@ -148,39 +148,24 @@ export class PackageDependenciesProvider implements vscode.TreeDataProvider<Tree
}
if (!element) {
const workspaceState = await folderContext.swiftPackage.loadWorkspaceState();
// Build PackageNodes for all dependencies. Because Package.resolved might not
// be up to date with edited dependency list, we need to remove the edited
// dependencies from the list before adding in the edit version
const children = [
...this.getLocalDependencies(workspaceState),
...this.getRemoteDependencies(folderContext),
];
const editedChildren = await this.getEditedDependencies(workspaceState);
const uneditedChildren: PackageNode[] = [];
for (const child of children) {
const editedVersion = editedChildren.find(item => item.name === child.name);
if (!editedVersion) {
uneditedChildren.push(child);
}
}
return [...uneditedChildren, ...editedChildren].sort((first, second) =>
first.name.localeCompare(second.name)
);
return this.getDependencyGraph(workspaceState, folderContext.folder.fsPath);
}

const buildDirectory = buildDirectoryFromWorkspacePath(folderContext.folder.fsPath, true);
return this.getNodesInDirectory(element.path);
}

if (element instanceof PackageNode) {
// Read the contents of a package.
const packagePath =
element.type === "remote"
? path.join(buildDirectory, "checkouts", getRepositoryName(element.path))
: element.path;
return this.getNodesInDirectory(packagePath);
} else {
// Read the contents of a directory within a package.
return this.getNodesInDirectory(element.path);
}
private getDependencyGraph(
workspaceState: WorkspaceState | undefined,
folderContext: string
): PackageNode[] {
return (
workspaceState?.object.dependencies.map(dependency => {
const type = this.dependencyType(dependency);
const version = this.dependencyDisplayVersion(dependency);
const packagePath = this.dependencyPackagePath(dependency, folderContext);
return new PackageNode(dependency.packageRef.identity, packagePath, version, type);
}) ?? []
);
}

/**
Expand Down Expand Up @@ -277,4 +262,78 @@ export class PackageDependenciesProvider implements vscode.TreeDataProvider<Tree
}
});
}

/// - Dependency display helpers

/**
* Get type of WorkspaceStateDependency for displaying in the tree: real version | edited | local
* @param dependency
* @return "local" | "remote" | "editing"
*/
private dependencyType(dependency: WorkspaceStateDependency): "local" | "remote" | "editing" {
if (dependency.state.name === "edited") {
return "editing";
} else if (
dependency.packageRef.kind === "local" ||
dependency.packageRef.kind === "fileSystem"
) {
// need to check for both "local" and "fileSystem" as swift 5.5 and earlier
// use "local" while 5.6 and later use "fileSystem"
return "local";
} else {
return "remote";
}
}
/**
* Get version of WorkspaceStateDependency for displaying in the tree
* @param dependency
* @return real version | editing | local
*/
private dependencyDisplayVersion(dependency: WorkspaceStateDependency): string {
const type = this.dependencyType(dependency);
if (type === "editing") {
if (dependency.basedOn) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a user is editing a package using a local version of the dependency ie they ran

swift package edit <dependency> -path <local-version-folder>

I don't think we can say it is based on a particular version. We can only say it is editing <version> when they have called

swift package edit <dependency>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

editing is ok to me no strong opinion about it

but I'm trying to understand the real use case of edit

When is the right scenario that we should use edit?

  • it has been a dependency already
  • it can't be a local dependency before you edit

So im thinking the scenario is that

  • If it's local dependency you can change whatever freely which is nothing about edit
  • if you edit remote dependency, it means the engineer wants to make some change on current remote version/branch, then they get a chance to fix the existing issues of current version (or switching other version? to check)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two versions of swift package edit are for different situations

  1. without a path is generally used for testing a quick change to a dependency. I just need an editable copy of the current version of that dependency. Including the version in the editing string makes sense here as your changes are based of a specific version of the dependency.
  2. With a path argument is used when I have been developing a feature in one of the dependencies independently of the current package and I want to test that new feature in the current package. I can then say I would rather use this version of my dependency over the version downloaded from the repository. In this case because there is no versioning link between the two versions of the dependency it doesn't make sense to include the version number in the editing string

Does that make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. fully understand these cases.
if we really want to show the editing version in the case 2, probably we should also show the real version of the dependency in case 1

Do we continue this PR to see the result. here I summarize a bit.

  1. show editing 1.2.3 for case 1, show editing for case 2
  2. show editing 1.2.3 for case 1 and case 2. needs to read info swift package describe
  3. leave them as editing. close this

Which one do you prefer.
If it's 3, I'd like to create another PR about showing menu of use locally by right click, so that swift package edit can be done in UI action

const basedOnVersion = this.dependencyDisplayVersion(dependency.basedOn);
if (basedOnVersion.length > 0) {
return "editing " + basedOnVersion;
}
}
return "editing";
} else if (type === "local") {
return "local";
} else {
return (
dependency.state.checkoutState?.version ??
dependency.state.checkoutState?.branch ??
dependency.state.checkoutState?.revision.substring(0, 7) ??
"unknown"
);
}
}

/**
* Get type of WorkspaceStateDependency for displaying in the tree: real version | edited | local
* `edited`: dependency.state.path ?? workspacePath + Packages/ + dependency.subpath
* `local`: dependency.packageRef.location
* `remote`: buildDirectory + checkouts + dependency.packageRef.location
* @param dependency
* @return the package path based on the type
*/
private dependencyPackagePath(
dependency: WorkspaceStateDependency,
workspaceFolder: string
): string {
const type = this.dependencyType(dependency);
if (type === "editing") {
return (
dependency.state.path ?? path.join(workspaceFolder, "Packages", dependency.subpath)
);
} else if (type === "local") {
return dependency.state.path ?? dependency.packageRef.location;
} else {
// remote
const buildDirectory = buildDirectoryFromWorkspacePath(workspaceFolder, true);
return path.join(buildDirectory, "checkouts", dependency.subpath);
}
}
}