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

Package View Improvements #175

Closed
3 of 4 tasks
giggsey opened this issue May 23, 2020 · 12 comments · Fixed by #426
Closed
3 of 4 tasks

Package View Improvements #175

giggsey opened this issue May 23, 2020 · 12 comments · Fixed by #426
Labels
enhancement New feature or request

Comments

@giggsey
Copy link
Contributor

giggsey commented May 23, 2020

  • A copy button for the package name (makes installation much easier)
  • Ability to view the README for a package
  • List of available releases
  • List of dependencies

Mimics our commonly used features from satis/packagist

@akondas akondas added the enhancement New feature or request label May 25, 2020
@nickygerritsen
Copy link
Contributor

I wanted to look into implementing this. Some questions / remarks:

  • For the copy button I guess we can use the existing clipboard.svg and add a small button after the name?
  • For the readme, available releases and dependencies, see below

For the readme: I think this is the hardest one, since we need to actually get the readme; this is not part of the composer PackageInterface. First of all, it's probably best to use the latest release, since we have more logic for that already. Then what we could do is use the existing PackageManager::distFilename function to find the zip/tar for that version and then check the contents. However:

  • How to handle tar files? I saw the recent SensioLabsPackageScanner only handles zip files so should we also do that here?
  • How to determine the filename of the readme file? We can have readme and readme.md (and with caps) and probably some more, so we should determine an order. Also, it could happen there is a readme in a subfolder. How do we handle that we only want the root one? My zip files have one root folder and then in there are the files, but I haven't found the logic of how that root folder is named yet. Then if it is markdown we can use https://github.com/KnpLabs/KnpMarkdownBundle to render it?
  • Where do we store the readme? Add a field to Buddy\Repman\Entity\Organization\Package?

For releases and dependencies, as well as the readme, I propose to add a OrganizationController::package action for the GET URL /organization/{organization}/package/{package} which can display all this info. Getting the versions and dependencies we can do by calling PackageManager::findProviders although I wonder if we should add a findProvider that takes one package?
Packagist shows links to required packages that also exist, we could do that if we add something like PackageRepository::findByNames? I'm not sure how you normally handle if you give it 5 names and only 3 exist. I only see that in DbalDownloadsQuery::findByNames but that's a bit different, it's not in a repository.

To make life even easier we could show a copy button after each version on this detail page, that will copy "my/package": "1.2.3" to the clipboard?

Any thoughts? We could also start with the simpler ones, like the copy button and the releases/dependencies.

@giggsey
Copy link
Contributor Author

giggsey commented May 29, 2020

I'm not sure how you normally handle if you give it 5 names and only 3 exist.

I think linking through to packagist for those non-local packages would be fine.

To make life even easier we could show a copy button after each version on this detail page, that will copy "my/package": "1.2.3" to the clipboard?

Most people (hopefully) would want to install that as a minimum version, so ideally "^1.2.3". Although with a copyable package link from the package view, that's less of an issue.

@nickygerritsen
Copy link
Contributor

I'm not sure how you normally handle if you give it 5 names and only 3 exist.

I think linking through to packagist for those non-local packages would be fine.

But it might also not be on Packagist but somewhere else?

To make life even easier we could show a copy button after each version on this detail page, that will copy "my/package": "1.2.3" to the clipboard?

Most people (hopefully) would want to install that as a minimum version, so ideally "^1.2.3". Although with a copyable package link from the package view, that's less of an issue.

Fair point, I think ^ makes sense.

@giggsey
Copy link
Contributor Author

giggsey commented May 29, 2020

I'm not sure how you normally handle if you give it 5 names and only 3 exist.

I think linking through to packagist for those non-local packages would be fine.

But it might also not be on Packagist but somewhere else?

Good point. Our local satis setup mirrors all of our dependencies (and we only use local or Packagist anyway), so it contains links for everything.

Maybe then it only links to internal packages it knows about. That does mean a full package list will need to be known on page load though

@nickygerritsen
Copy link
Contributor

Yes, but that is stored in the database in one table anyway. I do wonder what the impact will be though if you have a lot of packages.

@giggsey
Copy link
Contributor Author

giggsey commented May 29, 2020 via email

@akondas
Copy link
Member

akondas commented Jun 1, 2020

This topic is getting a bit extensive so if in doubt I suggest you do a separate issue. I would not like someone to work on something that we do not merge 😉.

When it comes to tar files, we will handle them, I still have this feature during the works, but I looked at this topic initially.

For releases and dependencies, as well as the readme, I propose to add a OrganizationController::package action for the GET URL /organization/{organization}/package/{package} which can display all this info. Getting the versions and dependencies we can do by calling PackageManager::findProviders although I wonder if we should add a findProvider that takes one package?

Sounds good. At the moment I would take what it is, there is no point in optimizing if there are no performance problems.

When it comes to readme detection, I would do some simple mechanism and improved it over time. The readme content could be pushed to the table to make displaying faster (read model).

nickygerritsen added a commit to Lets-Talk-NL/repman that referenced this issue Jul 4, 2020
nickygerritsen added a commit to Lets-Talk-NL/repman that referenced this issue Jul 4, 2020
nickygerritsen added a commit to Lets-Talk-NL/repman that referenced this issue Jul 4, 2020
nickygerritsen added a commit to Lets-Talk-NL/repman that referenced this issue Jul 4, 2020
nickygerritsen added a commit to Lets-Talk-NL/repman that referenced this issue Jul 4, 2020
nickygerritsen added a commit to Lets-Talk-NL/repman that referenced this issue Jul 4, 2020
nickygerritsen added a commit to Lets-Talk-NL/repman that referenced this issue Jul 4, 2020
nickygerritsen added a commit to Lets-Talk-NL/repman that referenced this issue Jul 4, 2020
nickygerritsen added a commit to Lets-Talk-NL/repman that referenced this issue Jul 4, 2020
nickygerritsen added a commit to Lets-Talk-NL/repman that referenced this issue Jul 4, 2020
nickygerritsen added a commit to Lets-Talk-NL/repman that referenced this issue Jul 4, 2020
nickygerritsen added a commit to Lets-Talk-NL/repman that referenced this issue Jul 4, 2020
nickygerritsen added a commit to Lets-Talk-NL/repman that referenced this issue Jul 6, 2020
nickygerritsen added a commit to Lets-Talk-NL/repman that referenced this issue Jul 6, 2020
nickygerritsen added a commit to Lets-Talk-NL/repman that referenced this issue Jul 6, 2020
nickygerritsen added a commit to Lets-Talk-NL/repman that referenced this issue Jul 6, 2020
nickygerritsen added a commit to Lets-Talk-NL/repman that referenced this issue Jul 6, 2020
nickygerritsen added a commit to Lets-Talk-NL/repman that referenced this issue Jul 7, 2020
nickygerritsen added a commit to Lets-Talk-NL/repman that referenced this issue Jul 7, 2020
@giggsey
Copy link
Contributor Author

giggsey commented Oct 14, 2020

I'm thinking about working on these last two features, showing the information for the latest version only.

Any objections?

@akondas
Copy link
Member

akondas commented Oct 15, 2020

Not on my part. You can go ahead and take action 😉 .
If it will only be for the latest version then I think we can add it to the package version model (but hydrate in a separate read model).

@giggsey
Copy link
Contributor Author

giggsey commented Mar 9, 2021

Just taking a look at the dependencies task, this is my current plan:

  • New table called organization_package_link
    • organization_id
    • package_id
    • target (package name)
    • constraint (e.g. ^1.5.0)
    • type (requires, devRequires, suggests etc (all the Composer Link types))
  • Composer Package Sync grabs populates these for the latest version
  • We remove all links on sync, then add the current ones back in
  • Display on the UI
  • Show the reverse on the UI too (what depends on this package)

If you're happy with that plan, I'll try working on it over the next few days/weeks.

@akondas
Copy link
Member

akondas commented Mar 11, 2021

I wonder if it really has to be a separate table? Will this data be searchable or cumulative with other tables? Because if not and it is only to be viewed on the UI, it seems to me that an additional json column would be enough.

@giggsey
Copy link
Contributor Author

giggsey commented Mar 11, 2021

I wonder if it really has to be a separate table? Will this data be searchable or cumulative with other tables? Because if not and it is only to be viewed on the UI, it seems to me that an additional json column would be enough.

There is a two way search. Each package will list its dependencies, and a separate list showing what depends on this package. If it was only listing dependencies, a JSON column would be okay.

I might have a draft MR within a few days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants