-
-
Notifications
You must be signed in to change notification settings - Fork 188
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
base: 8.4
Are you sure you want to change the base?
FEATURE: Improve flow:package:list command #3363
Conversation
What do you think about to write instead of |
I would also re-add the option about the loading order |
I thought With: This is why i was a bit confused and why i deleted this parameter 🤔 |
bba3cee
to
309770a
Compare
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: |
I would actually say lets do only loading order? Who cares about anything else? |
Thanks @ahaeslich ! I hadn't actually tried it that way 🤦🏼♂️ @kitsunet you mean without the user input Option to say loadingOrder=true? |
Yes, the default order is waht? case-sensitive alphabetic? seems not useful for anything. |
By default Then I would suggest we take a vote. 👍🏼: loadingOrder should be allowed to be set to true by the user. |
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? |
Yes |
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 :) |
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.) |
As discussed with @crydotsnake we will not show a column but keep the star If package freezing works or not is almost irrelevant and even more a reason to remove it ^^ |
$frozenPackages = []; | ||
$longestPackageKey = 0; | ||
$availablePackages = $this->packageManager->getAvailablePackages(); | ||
ksort($availablePackages); |
There was a problem hiding this comment.
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?
ksort($availablePackages); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes 🤦🏽
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
IMO when we list the packages by loading order, we should also do the same for the backend module. |
yes i guess but that would be part of a neos change |
Yes 👍🏼 |
@crydotsnake If you don't know how the set a argument in the cli, you can use the help command. Example: will you return this
|
ab2946b
to
2d0f9fe
Compare
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.Checklist
created, run andadjustedas neededFEATURE|TASK|BUGFIX
!!!
and have upgrade-instructions