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

FEATURE: Improve flow:package:list command #3363

Open
wants to merge 4 commits into
base: 8.4
Choose a base branch
from

Conversation

crydotsnake
Copy link
Member

Upgrade instructions

None

Review instructions

Currently the target branch is 8.3. But i will change the target branch later to 8.4 if the branch is available. Because of that i will open the PR as a DRAFT first.

This PR improves the view for the flow:package:list command. The Installed packages with the installed version, and frozen state are now displayed in a table for a better detailed view.

WindowsTerminal_8KgC2tH4ee

Checklist

  • Code follows the PSR-2 coding style
  • Tests have been created, run and adjusted as needed
  • The PR is created against the lowest maintained branch
  • Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
  • Reviewer - The first section explains the change briefly for change-logs
  • Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

@crydotsnake crydotsnake marked this pull request as draft June 8, 2024 07:06
@github-actions github-actions bot added the 8.3 label Jun 8, 2024
@crydotsnake crydotsnake removed the 8.3 label Jun 8, 2024
@jonnitto
Copy link
Member

jonnitto commented Jun 8, 2024

What do you think about to write instead of Not Fozen a and for Frozen a ?
Like that, the screen would be less cluttered with the same words

@jonnitto
Copy link
Member

jonnitto commented Jun 8, 2024

I would also re-add the option about the loading order

@crydotsnake
Copy link
Member Author

crydotsnake commented Jun 8, 2024

I would also re-add the option about the loading order

I thought $loadingOrder is always false and can therfore not be changed by the user in the input.

With: ./flow flow:package:list true or ./flow flow:package:list loadingOrder=true the order is still the same 🤔

This is why i was a bit confused and why i deleted this parameter 🤔

@ahaeslich
Copy link
Member

I would also re-add the option about the loading order

I second that. Can't tell you how many times I used this option to show people during onboarding or debugging why some settings were not set as expected. This is indeed a very important flag in my opinion.

Btw. the usage would be: ./flow package:list --loading-order

@kitsunet
Copy link
Member

I would actually say lets do only loading order? Who cares about anything else?

@crydotsnake
Copy link
Member Author

Thanks @ahaeslich !

I hadn't actually tried it that way 🤦🏼‍♂️

@kitsunet you mean without the user input Option to say loadingOrder=true?

@kitsunet
Copy link
Member

Yes, the default order is waht? case-sensitive alphabetic? seems not useful for anything.

@crydotsnake
Copy link
Member Author

crydotsnake commented Jun 11, 2024

By default ksort() sorts the array in ascending order If I'm not mistaken.

Then I would suggest we take a vote.

👍🏼: loadingOrder should be allowed to be set to true by the user.
👎🏻: loadingOrder should not be allowed to be set to true by the user.

@ahaeslich
Copy link
Member

I would actually say lets do only loading order? Who cares about anything else?

This briefly crossed my mind. Should have asked then, why we even would need an unordered list 😁.

Just a second thought: Are we sorting the package list by loading order in the backend module? Should we?

@crydotsnake
Copy link
Member Author

Just a second thought: Are we sorting the package list by loading order in the backend module?

Yes

@mhsdesign
Copy link
Member

mhsdesign commented Jun 11, 2024

So my two cents (just a proposal):

Make sort by loading order the default (and remove the flag as it will be obsolete)

Regarding frozen vs not frozen. In slack we already agreed that this is a relict of the past, so unless there is any package frozen at all the indicator can be dropped by default. (So no frozen column and if there is at least one package frozen add a column and show the distinction)

With the removal of frozen we would then eventually get rid of it for real :)

Also i would found it really helpful to show the composer name as well :)

@crydotsnake
Copy link
Member Author

crydotsnake commented Jun 12, 2024

and if there is at least one package frozen add a column and show the distinction)

Currently, when you freeze one package eg. Flowpack.Media.Ui. It freezes ALL packages. I´m not sure if this is intentional/wanted.

I could imagine that it also freezes the dependency packages. But then I find it strange why, for example, the Symfony packages are also frozen with the Flowpack Media UI.

Also i would say it cant hurt to show the column by default with a indicator that none of the packages is frozen. IMO it doesent make sense to only show this column when there is one, or more packages that are frozen. I don't see the point behind it tbh. 🙂 (Correct me if I misunderstood you.)

@mhsdesign
Copy link
Member

As discussed with @crydotsnake we will not show a column but keep the star * indicator which is considered temporary and will be dropped soon.

If package freezing works or not is almost irrelevant and even more a reason to remove it ^^

@github-actions github-actions bot added the 8.3 label Jun 13, 2024
@crydotsnake crydotsnake changed the base branch from 8.3 to 8.4 June 14, 2024 13:38
@crydotsnake crydotsnake marked this pull request as ready for review June 14, 2024 13:38
$frozenPackages = [];
$longestPackageKey = 0;
$availablePackages = $this->packageManager->getAvailablePackages();
ksort($availablePackages);
Copy link
Member

Choose a reason for hiding this comment

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

i thought we decided to only show them in loading order?

Suggested change
ksort($availablePackages);

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes 🤦🏽

Copy link
Member Author

Choose a reason for hiding this comment

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

But then i would say we should change the loading order in the backend module too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to keep it consistent. Wdyt @mhsdesign

@crydotsnake
Copy link
Member Author

IMO when we list the packages by loading order, we should also do the same for the backend module.

@mhsdesign
Copy link
Member

yes i guess but that would be part of a neos change

@crydotsnake
Copy link
Member Author

Yes 👍🏼

@jonnitto
Copy link
Member

@crydotsnake If you don't know how the set a argument in the cli, you can use the help command.

Example: ./flow help package:list

will you return this

List available packages

COMMAND:
  neos.flow:package:list

USAGE:
  ./flow package:list [<options>]

OPTIONS:
  --loading-order      The returned packages are ordered by their loading
                       order.

DESCRIPTION:
  Lists all locally available packages. Displays the package key, version and
  package title.

@crydotsnake crydotsnake force-pushed the feature/improve-package-list-command branch from ab2946b to 2d0f9fe Compare September 1, 2024 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants