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

Different format for listing suggested packages on install #6267

Closed
snoopdouglas opened this issue Mar 15, 2017 · 6 comments
Closed

Different format for listing suggested packages on install #6267

snoopdouglas opened this issue Mar 15, 2017 · 6 comments
Assignees
Labels
Milestone

Comments

@snoopdouglas
Copy link

@snoopdouglas snoopdouglas commented Mar 15, 2017

The format for listing suggested packages features a lot of repetition and is a tad verbose. I find it hard to read, so I'd like to suggest a different format.

For example, here's the tail of the output when starting a new Craft 3 Beta project:

$ composer create-project craftcms/craft . -s beta

...

zendframework/zend-feed suggests installing zendframework/zend-cache (Zend\Cache component, for optionally caching feeds between requests)
zendframework/zend-feed suggests installing zendframework/zend-db (Zend\Db component, for use with PubSubHubbub)
zendframework/zend-feed suggests installing zendframework/zend-http (Zend\Http for PubSubHubbub, and optionally for use with Zend\Feed\Reader)
zendframework/zend-feed suggests installing zendframework/zend-servicemanager (Zend\ServiceManager component, for easily extending ExtensionManager implementations)
zendframework/zend-feed suggests installing zendframework/zend-validator (Zend\Validator component, for validating email addresses used in Atom feeds and entries ehen using the Writer subcomponent)
league/flysystem suggests installing league/flysystem-aws-s3-v2 (Allows you to use S3 storage with AWS SDK v2)
league/flysystem suggests installing league/flysystem-aws-s3-v3 (Allows you to use S3 storage with AWS SDK v3)
league/flysystem suggests installing league/flysystem-azure (Allows you to use Windows Azure Blob storage)
league/flysystem suggests installing league/flysystem-cached-adapter (Flysystem adapter decorator for metadata caching)
league/flysystem suggests installing league/flysystem-copy (Allows you to use Copy.com storage)
league/flysystem suggests installing league/flysystem-dropbox (Allows you to use Dropbox storage)
league/flysystem suggests installing league/flysystem-eventable-filesystem (Allows you to use EventableFilesystem)
league/flysystem suggests installing league/flysystem-rackspace (Allows you to use Rackspace Cloud Files)
league/flysystem suggests installing league/flysystem-sftp (Allows you to use SFTP server storage via phpseclib)
league/flysystem suggests installing league/flysystem-webdav (Allows you to use WebDAV storage)
league/flysystem suggests installing league/flysystem-ziparchive (Allows you to use ZipArchive adapter)
pixelandtonic/imagine suggests installing ext-imagick (to use the Imagick implementation)
pixelandtonic/imagine suggests installing ext-gmagick (to use the Gmagick implementation)
craftcms/cms suggests installing ext-imagick (Adds support for more image processing formats and options.)
craftcms/cms suggests installing ext-intl (Adds rich internationalization support.)

If these suggestions were instead organised by the suggesting package, and left out the package descriptions (perhaps unless a verbose flag is set), we'd end up with something like this:

zendframework/zend-feed suggests installing:
  - zendframework/zend-cache
  - zendframework/zend-db
  - zendframework/zend-http
  - zendframework/zend-servicemanager
  - zendframework/zend-validator

league/flysystem suggests installing:
  - league/flysystem-aws-s3-v2
  - league/flysystem-aws-s3-v3
  - league/flysystem-azure
  - league/flysystem-cached-adapter
  - league/flysystem-copy
  - league/flysystem-dropbox
  - league/flysystem-eventable-filesystem
  - league/flysystem-rackspace
  - league/flysystem-sftp
  - league/flysystem-webdav
  - league/flysystem-ziparchive

pixelandtonic/imagine suggests installing:
  - ext-imagick
  - ext-gmagick

craftcms/cms suggests installing:
  - ext-imagick
  - ext-intl

We could condense this even further using space-delimited lists (similar to apt):

zendframework/zend-feed suggests installing:
  zendframework/zend-cache zendframework/zend-db zendframework/zend-http
  zendframework/zend-servicemanager zendframework/zend-validator

league/flysystem suggests installing:
  league/flysystem-aws-s3-v2 league/flysystem-aws-s3-v3 league/flysystem-azure
  league/flysystem-cached-adapter league/flysystem-copy league/flysystem-dropbox
  league/flysystem-eventable-filesystem league/flysystem-rackspace
  league/flysystem-sftp league/flysystem-webdav league/flysystem-ziparchive

pixelandtonic/imagine suggests installing:
  ext-imagick ext-gmagick

craftcms/cms suggests installing:
  ext-imagick ext-intl
@curry684

This comment has been minimized.

Copy link
Contributor

@curry684 curry684 commented Mar 15, 2017

The problem is not in the format but in the usage. Packages should only suggest packages if that improves their functionality, like performance or reliability. It's meant to be a "I can work without this package but SO MUCH BETTER WITH". Flysystem suggesting 20 adapters of which you'll likely not want to include 18 or 19 is the cause of the spam, not the suggest markup. They're just optional, and the required dependency the other way around declares that relationship more than enough.

I would even go as far that a suggestion is only valid if it will be automatically used if installed, like a caching system. If any step is required it's just the developer's choice.

Also, you definitely do not want to hide the reason by default. Without the reason everyone will just ignore it, just like with apt, because noone tells you why you should bother.

@snoopdouglas

This comment has been minimized.

Copy link
Author

@snoopdouglas snoopdouglas commented Mar 16, 2017

Perhaps the distinction should be made between recommendation and suggestion then - again like apt?

If these lists were far shorter, I'd agree that including the descriptions would be a good thing.

@alcohol

This comment has been minimized.

Copy link
Member

@alcohol alcohol commented Mar 17, 2017

Considering it is both possible to omit this list from being output during install/update, or to call it as a stand-alone command, I see no reason to really change anything here.

@alcohol alcohol added the Question label Mar 17, 2017
@Seldaek

This comment has been minimized.

Copy link
Member

@Seldaek Seldaek commented Mar 17, 2017

I think the output is fine but yeah on initial install it's kinda useless as it spams with suggestions of everything.. maybe we should only output if you had already one package installed, or like during install output max 5-10 rows and then a line saying for more use the suggests command. Something to consider.

@Seldaek Seldaek added this to the 2.0 milestone Mar 17, 2017
@curry684

This comment has been minimized.

Copy link
Contributor

@curry684 curry684 commented Mar 18, 2017

Looking at https://packagist.org/packages/league/flysystem as a prime example of my point of 'suggestions' being abused: it would be a good idea perhaps to just list suggested platform packages. What you frequently see on that end is packages that simply have more, faster or better functionality if Imagick, BCmath, native encryption etc. are available. For Flysystem actual functionality is added to the package itself if ext-fileinfo is present.

For regular packages, in my experience, suggestions usually boil down to listing reverse dependencies. I just don't give a damn that there's a Flysystem module for Azure when I'm using S3. During install and update that could indeed be condensed to a oneliner pointing me to the suggests command for listing 23 suggested packages.

@Seldaek Seldaek self-assigned this Feb 2, 2020
@Seldaek Seldaek modified the milestones: 2.x, 2.0-core Feb 2, 2020
Seldaek added a commit that referenced this issue Feb 13, 2020
…reuse SuggestedPackagesReporter and make smarter defaults, fixes #6267
@Seldaek

This comment has been minimized.

Copy link
Member

@Seldaek Seldaek commented Feb 13, 2020

Fixed by 44d1e15

@Seldaek Seldaek closed this Feb 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.