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

How old deps #255

Merged
merged 6 commits into from
Oct 8, 2024
Merged

How old deps #255

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9509388
[backend] add `highestVersion` to PackageSummary
ZenVoich Oct 7, 2024
ca9f309
update declarations
ZenVoich Oct 7, 2024
4483c3e
[frontend] show how old is dependency
ZenVoich Oct 7, 2024
9918e5f
[backend] include dependents only if latest version depends
ZenVoich Oct 7, 2024
786b183
[frontend] show on which version depends
ZenVoich Oct 7, 2024
9d6c49f
-
ZenVoich Oct 7, 2024
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
2 changes: 1 addition & 1 deletion backend/main/registry/getPackageDetails.mo
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ module {
dependent or devDependent;
};

let dependentConfigs = Array.filter<PackageConfigV3>(registry.getAllConfigs(), isDependent);
let dependentConfigs = Array.filter<PackageConfigV3>(registry.getHighestConfigs(), isDependent);

let pkgHash = func(a : PackageConfigV3) : Hash.Hash {
Text.hash(a.name);
Expand Down
3 changes: 3 additions & 0 deletions backend/main/registry/getPackageSummary.mo
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ module {
let owner = registry.getPackageOwner(name)!;
users.ensureUser(owner);

let highestVersion = registry.getHighestVersion(name)!;

Comment on lines +31 to +32
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Consider adding error handling for getHighestVersion

The use of the ! operator assumes that registry.getHighestVersion(name) will always return a value. This might lead to runtime errors if the highest version cannot be retrieved for any reason.

Consider using pattern matching or the do? syntax to handle potential null values more gracefully. For example:

-let highestVersion = registry.getHighestVersion(name)!;
+let highestVersion = switch (registry.getHighestVersion(name)) {
+  case (null) { return null; };
+  case (?version) { version };
+};

This approach ensures that the function returns null if the highest version cannot be retrieved, maintaining consistency with the function's return type of ?PackageSummary.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
let highestVersion = registry.getHighestVersion(name)!;
let highestVersion = switch (registry.getHighestVersion(name)) {
case (null) { return null; };
case (?version) { version };
};

return ?{
depAlias = "";
owner = owner;
Expand All @@ -38,6 +40,7 @@ module {
downloadsInLast30Days = downloadLog.getDownloadsByPackageNameIn(config.name, 30 * DAY, Time.now());
downloadsTotal = downloadLog.getTotalDownloadsByPackageName(config.name);
quality = _computePackageQuality(registry, name, version);
highestVersion = highestVersion;
};
};
};
Expand Down
1 change: 1 addition & 0 deletions backend/main/types.mo
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ module {
downloadsInLast7Days : Nat;
quality : PackageQuality;
depAlias : Text; // dep alias (e.g. "lib@1" or "lib")
highestVersion : PackageVersion;
};

public type PackageSummaryWithChanges = PackageSummary and {
Expand Down
26 changes: 16 additions & 10 deletions cli/declarations/main/main.did
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,13 +91,13 @@ type Result_6 =
err: Err;
ok: vec record {
PackageName;
PackageVersion;
PackageVersion__1;
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Incomplete Renaming of PackageVersion Detected

Multiple instances of PackageVersion without the __1 suffix were found in the following files:

  • ./frontend/declarations/main/main.did
    • Line X
    • Line Y
    • ...
  • ./cli/declarations/main/main.did
    • Line X
    • Line Y
    • ...

Please ensure all occurrences are renamed to PackageVersion__1 to maintain consistency across the codebase.

🔗 Analysis chain

LGTM. Verify consistency across the codebase.

The change from PackageVersion to PackageVersion__1 in the Result_6 type is correct and consistent with the renaming throughout the file.

To ensure all occurrences have been updated, run the following command:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining instances of PackageVersion (without __1)
rg --type candid 'PackageVersion(?!__1)' .

Length of output: 372

Script:

#!/bin/bash
# Search for any instances of PackageVersion not followed by __1
rg --type candid 'PackageVersion' . | grep -v 'PackageVersion__1'

Length of output: 94

Script:

#!/bin/bash
# Search for any instances of PackageVersion not followed by __1 in .did files
rg 'PackageVersion' --glob '*.did' . | grep -v 'PackageVersion__1'

Length of output: 896

};
};
type Result_5 =
variant {
err: Err;
ok: PackageVersion;
ok: PackageVersion__1;
};
type Result_4 =
variant {
Expand Down Expand Up @@ -147,6 +147,7 @@ type Request =
};
type PublishingId = text;
type PageCount = nat;
type PackageVersion__1 = text;
type PackageVersion = text;
Comment on lines +150 to 151
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Consider removing the original PackageVersion type.

While the addition of PackageVersion__1 is correct, keeping the original PackageVersion type might lead to confusion and inconsistency in the codebase.

Consider applying this change:

 type PackageVersion__1 = text;
-type PackageVersion = text;

This will help maintain consistency with the renaming strategy and prevent potential confusion or errors in the future.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
type PackageVersion__1 = text;
type PackageVersion = text;
type PackageVersion__1 = text;

type PackageSummary__1 =
record {
Expand All @@ -155,6 +156,7 @@ type PackageSummary__1 =
downloadsInLast30Days: nat;
downloadsInLast7Days: nat;
downloadsTotal: nat;
highestVersion: PackageVersion;
ZenVoich marked this conversation as resolved.
Show resolved Hide resolved
owner: principal;
ownerInfo: User;
publication: PackagePublication;
Expand All @@ -168,6 +170,7 @@ type PackageSummaryWithChanges__1 =
downloadsInLast30Days: nat;
downloadsInLast7Days: nat;
downloadsTotal: nat;
highestVersion: PackageVersion;
owner: principal;
ownerInfo: User;
publication: PackagePublication;
Expand All @@ -181,6 +184,7 @@ type PackageSummaryWithChanges =
downloadsInLast30Days: nat;
downloadsInLast7Days: nat;
downloadsTotal: nat;
highestVersion: PackageVersion;
owner: principal;
ownerInfo: User;
publication: PackagePublication;
Expand All @@ -193,6 +197,7 @@ type PackageSummary =
downloadsInLast30Days: nat;
downloadsInLast7Days: nat;
downloadsTotal: nat;
highestVersion: PackageVersion;
owner: principal;
ownerInfo: User;
publication: PackagePublication;
Expand Down Expand Up @@ -237,6 +242,7 @@ type PackageDetails =
downloadsInLast7Days: nat;
downloadsTotal: nat;
fileStats: PackageFileStatsPublic;
highestVersion: PackageVersion;
ZenVoich marked this conversation as resolved.
Show resolved Hide resolved
owner: principal;
ownerInfo: User;
publication: PackagePublication;
Expand Down Expand Up @@ -303,13 +309,13 @@ type Main =
getDefaultPackages: (text) ->
(vec record {
PackageName;
PackageVersion;
PackageVersion__1;
}) query;
getDownloadTrendByPackageId: (PackageId) ->
(vec DownloadsSnapshot__1) query;
getDownloadTrendByPackageName: (PackageName) ->
(vec DownloadsSnapshot__1) query;
getFileHashes: (PackageName, PackageVersion) -> (Result_8);
getFileHashes: (PackageName, PackageVersion__1) -> (Result_8);
getFileHashesByPackageIds: (vec PackageId) ->
(vec record {
PackageId;
Expand All @@ -318,19 +324,19 @@ type Main =
blob;
};
});
getFileHashesQuery: (PackageName, PackageVersion) -> (Result_8) query;
getFileIds: (PackageName, PackageVersion) -> (Result_7) query;
getFileHashesQuery: (PackageName, PackageVersion__1) -> (Result_8) query;
getFileIds: (PackageName, PackageVersion__1) -> (Result_7) query;
getHighestSemverBatch:
(vec record {
PackageName;
PackageVersion;
PackageVersion__1;
SemverPart;
}) -> (Result_6) query;
getHighestVersion: (PackageName) -> (Result_5) query;
getMostDownloadedPackages: () -> (vec PackageSummary) query;
getMostDownloadedPackagesIn7Days: () -> (vec PackageSummary) query;
getNewPackages: () -> (vec PackageSummary) query;
getPackageDetails: (PackageName, PackageVersion) -> (Result_4) query;
getPackageDetails: (PackageName, PackageVersion__1) -> (Result_4) query;
getPackagesByCategory: () -> (vec record {
text;
vec PackageSummary;
Expand All @@ -344,10 +350,10 @@ type Main =
getTotalPackages: () -> (nat) query;
getUser: (principal) -> (opt User__1) query;
http_request: (Request) -> (Response) query;
notifyInstall: (PackageName, PackageVersion) -> () oneway;
notifyInstall: (PackageName, PackageVersion__1) -> () oneway;
notifyInstalls: (vec record {
PackageName;
PackageVersion;
PackageVersion__1;
}) -> () oneway;
restore: (nat) -> ();
search: (Text, opt nat, opt nat) -> (vec PackageSummary, PageCount) query;
Expand Down
29 changes: 19 additions & 10 deletions cli/declarations/main/main.did.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ export interface Main {
'getBackupCanisterId' : ActorMethod<[], Principal>,
'getDefaultPackages' : ActorMethod<
[string],
Array<[PackageName, PackageVersion]>
Array<[PackageName, PackageVersion__1]>
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Inconsistent use of 'PackageVersion' and 'PackageVersion__1'

The code introduces PackageVersion__1 as a new type but continues to use PackageVersion in other parts of the codebase. This inconsistency appears in method signatures and type definitions, which may lead to confusion and potential type errors. It's important to maintain consistency in type usage throughout the code.

Affected lines:

  • Line 66: getDefaultPackages
  • Line 76: getFileHashes
  • Lines 81-84: getFileHashesQuery
  • Line 85: getFileIds
  • Line 87: getHighestSemverBatch
  • Line 94: getPackageDetails
  • Line 108: notifyInstall
  • Lines 110-112: notifyInstalls
  • Line 302: Result_5
  • Line 304: Result_6

Recommendation:

If PackageVersion__1 is intended to replace PackageVersion, consider updating all instances of PackageVersion to PackageVersion__1. Otherwise, if they represent different concepts, use more descriptive type names to differentiate them clearly.

// Example of updating types for consistency:

- export type PackageVersion = string;
+ // export type PackageVersion = string; (if deprecated)

- export type PackageVersion__1 = string;
+ export type PackageVersion = string;

// Update method signatures accordingly:
- 'getDefaultPackages' : ActorMethod<[string], Array<[PackageName, PackageVersion__1]>>,
+ 'getDefaultPackages' : ActorMethod<[string], Array<[PackageName, PackageVersion]>>,

Also applies to: 76-76, 81-84, 85-85, 87-87, 94-94, 108-108, 110-112, 302-302, 304-304

>,
'getDownloadTrendByPackageId' : ActorMethod<
[PackageId],
Expand All @@ -73,22 +73,25 @@ export interface Main {
[PackageName],
Array<DownloadsSnapshot__1>
>,
'getFileHashes' : ActorMethod<[PackageName, PackageVersion], Result_8>,
'getFileHashes' : ActorMethod<[PackageName, PackageVersion__1], Result_8>,
'getFileHashesByPackageIds' : ActorMethod<
[Array<PackageId>],
Array<[PackageId, Array<[FileId, Uint8Array | number[]]>]>
>,
'getFileHashesQuery' : ActorMethod<[PackageName, PackageVersion], Result_8>,
'getFileIds' : ActorMethod<[PackageName, PackageVersion], Result_7>,
'getFileHashesQuery' : ActorMethod<
[PackageName, PackageVersion__1],
Result_8
>,
'getFileIds' : ActorMethod<[PackageName, PackageVersion__1], Result_7>,
'getHighestSemverBatch' : ActorMethod<
[Array<[PackageName, PackageVersion, SemverPart]>],
[Array<[PackageName, PackageVersion__1, SemverPart]>],
Result_6
>,
'getHighestVersion' : ActorMethod<[PackageName], Result_5>,
'getMostDownloadedPackages' : ActorMethod<[], Array<PackageSummary>>,
'getMostDownloadedPackagesIn7Days' : ActorMethod<[], Array<PackageSummary>>,
'getNewPackages' : ActorMethod<[], Array<PackageSummary>>,
'getPackageDetails' : ActorMethod<[PackageName, PackageVersion], Result_4>,
'getPackageDetails' : ActorMethod<[PackageName, PackageVersion__1], Result_4>,
'getPackagesByCategory' : ActorMethod<
[],
Array<[string, Array<PackageSummary>]>
Expand All @@ -102,9 +105,9 @@ export interface Main {
'getTotalPackages' : ActorMethod<[], bigint>,
'getUser' : ActorMethod<[Principal], [] | [User__1]>,
'http_request' : ActorMethod<[Request], Response>,
'notifyInstall' : ActorMethod<[PackageName, PackageVersion], undefined>,
'notifyInstall' : ActorMethod<[PackageName, PackageVersion__1], undefined>,
'notifyInstalls' : ActorMethod<
[Array<[PackageName, PackageVersion]>],
[Array<[PackageName, PackageVersion__1]>],
undefined
>,
'restore' : ActorMethod<[bigint], undefined>,
Expand Down Expand Up @@ -182,6 +185,7 @@ export interface PackageDetails {
'deps' : Array<PackageSummary__1>,
'quality' : PackageQuality,
'testStats' : TestStats__1,
'highestVersion' : PackageVersion,
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Type mismatch in 'highestVersion' field across interfaces

The highestVersion field is added to multiple interfaces using PackageVersion as its type. However, other parts of the code are using PackageVersion__1. This inconsistency can cause type mismatches when the highestVersion field interacts with methods expecting PackageVersion__1.

Affected interfaces:

  • Line 188: PackageDetails
  • Line 228: PackageSummary
  • Line 240: PackageSummaryWithChanges
  • Line 253: PackageSummaryWithChanges__1
  • Line 266: PackageSummary__1

Recommendation:

Align the type of the highestVersion field with the rest of the codebase by using PackageVersion__1, or standardize on PackageVersion if appropriate.

// Example of updating the 'highestVersion' field:

- 'highestVersion' : PackageVersion,
+ 'highestVersion' : PackageVersion__1,

Also applies to: 228-228, 240-240, 253-253, 266-266

'downloadsTotal' : bigint,
'downloadsInLast30Days' : bigint,
'downloadTrend' : Array<DownloadsSnapshot>,
Expand Down Expand Up @@ -221,6 +225,7 @@ export interface PackageSummary {
'owner' : Principal,
'depAlias' : string,
'quality' : PackageQuality,
'highestVersion' : PackageVersion,
'downloadsTotal' : bigint,
'downloadsInLast30Days' : bigint,
'downloadsInLast7Days' : bigint,
Expand All @@ -232,6 +237,7 @@ export interface PackageSummaryWithChanges {
'owner' : Principal,
'depAlias' : string,
'quality' : PackageQuality,
'highestVersion' : PackageVersion,
'downloadsTotal' : bigint,
'downloadsInLast30Days' : bigint,
'downloadsInLast7Days' : bigint,
Expand All @@ -244,6 +250,7 @@ export interface PackageSummaryWithChanges__1 {
'owner' : Principal,
'depAlias' : string,
'quality' : PackageQuality,
'highestVersion' : PackageVersion,
'downloadsTotal' : bigint,
'downloadsInLast30Days' : bigint,
'downloadsInLast7Days' : bigint,
Expand All @@ -256,13 +263,15 @@ export interface PackageSummary__1 {
'owner' : Principal,
'depAlias' : string,
'quality' : PackageQuality,
'highestVersion' : PackageVersion,
'downloadsTotal' : bigint,
'downloadsInLast30Days' : bigint,
'downloadsInLast7Days' : bigint,
'config' : PackageConfigV3,
'publication' : PackagePublication,
}
export type PackageVersion = string;
export type PackageVersion__1 = string;
export type PageCount = bigint;
export type PublishingId = string;
export interface Request {
Expand Down Expand Up @@ -290,9 +299,9 @@ export type Result_3 = { 'ok' : FileId } |
{ 'err' : Err };
export type Result_4 = { 'ok' : PackageDetails } |
{ 'err' : Err };
export type Result_5 = { 'ok' : PackageVersion } |
export type Result_5 = { 'ok' : PackageVersion__1 } |
{ 'err' : Err };
export type Result_6 = { 'ok' : Array<[PackageName, PackageVersion]> } |
export type Result_6 = { 'ok' : Array<[PackageName, PackageVersion__1]> } |
{ 'err' : Err };
export type Result_7 = { 'ok' : Array<FileId> } |
{ 'err' : Err };
Expand Down
Loading