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.

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

Hide retired packages #24

Merged
merged 13 commits into from
Feb 19, 2024
Merged

Hide retired packages #24

merged 13 commits into from
Feb 19, 2024

Conversation

ffigiel
Copy link
Contributor

@ffigiel ffigiel commented Oct 7, 2023

Closes #18

This PR adds two things:

  • Retired versions will not be selected when syncing packages
  • The syncing process will try to delete packages when no valid versions are present

@ffigiel
Copy link
Contributor Author

ffigiel commented Oct 7, 2023

Running codegen changed the generated sql.gleam file a lot, I'm not sure if it's intended

@ffigiel
Copy link
Contributor Author

ffigiel commented Oct 7, 2023

I'm not sure if this behavior is what we want, and I should improve error handling

@ffigiel ffigiel marked this pull request as ready for review January 26, 2024 18:29
Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Can we hide them rather than deleting them please? 🙏

Weird how the ordering of the functions in the generated module has changed. Maybe we should make it sort them by name to avoid this in future.

@ffigiel
Copy link
Contributor Author

ffigiel commented Jan 29, 2024

@lpil I just found that there is a table named hidden_packages, should I add an entry there when all releases of a package are retired? What if a new release is made and it's not retired, should I remove the package from the hidden_packages then?

Before this discovery, I was planning to add an is_retired column to the packages table and use that, but maybe it's not necessary?

@lpil
Copy link
Member

lpil commented Jan 29, 2024

That table is for packages that have been deliberately hidden by us rather than retired by the package maintainer. I think it would be tricky to make that table mutable and not accidentally remove its contents thinking they are not retired.

Let's use the retirement field on releases. I don't think we'll have performance problems querying both tables, so we can avoid making a cache on the packages table for now.

@ffigiel
Copy link
Contributor Author

ffigiel commented Jan 29, 2024

Oh, I did not realize we had the retirement state in the database. This will make things easier 🙂
I have an initial version up, but it seems my query does not behave as expected (e.g. gleam_erlang is not shown anymore). I'll fix that and add some more tests
edit: Ready!

sql/get_package.sql Outdated Show resolved Hide resolved
@ffigiel ffigiel requested a review from lpil January 29, 2024 20:20
max(inserted_in_hex_at)
)
where
retirement_reason is not null;
Copy link
Member

Choose a reason for hiding this comment

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

Could you instead make a non_retired_packages view? I think that would be easier to use and it wouldn't have to read the entire table to filter a single package. Thank you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is done! (I learned some new things about sqlite along the way but it seems to work).

I had to adjust the text search query because sqlite views don't have rowids, but the search seems to behave the same way as it does in production.

Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Thank you. One small thing below

@@ -141,6 +141,25 @@ create table if not exists hidden_packages (
name text primary key
) strict;

create view if not exists non_retired_packages as
-- A package is retired if its latest release is retired
Copy link
Member

Choose a reason for hiding this comment

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

A package is retired if all its releases are retired. Something like this:

create view non_retired_packages as
select
  p.id
, p.name
, p.description
, p.docs_url
, p.links
, p.updated_in_hex_at
from packages p
where p.id in (
  select distinct r.package_id
  from releases r
  where r.id is null or r.retirement_reason is null
);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops! Fixed, thanks for the explanation

@ffigiel ffigiel marked this pull request as draft February 15, 2024 21:02
@ffigiel ffigiel marked this pull request as ready for review February 15, 2024 21:02
Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Thank you!

@lpil lpil merged commit 92e064f into gleam-lang:main Feb 19, 2024
1 check passed
@ffigiel ffigiel deleted the feat/hide-retired branch February 19, 2024 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hide retired packages
2 participants