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

Move shell-alias to an annotated command. #2376

Merged
merged 8 commits into from
Oct 15, 2016
Merged

Move shell-alias to an annotated command. #2376

merged 8 commits into from
Oct 15, 2016

Conversation

weitzman
Copy link
Member

@weitzman weitzman commented Oct 4, 2016

Todo:

  • How do we name a command shell-alias (no hyphen in function name)
  • The output formatter needs work. Can we keep the key-value listing that we have in 8.x? If not, should we use a table?
  • The default table formatting has dashed lines above and below the data. Thats a bit undesireable IMO. Our traditional tables dont have that
  • Drush tables always print the column name at top by default. I'd like to keep that. In this case it would be OK to omit them but I think they are helpful in general.
  • drush shell-alias --format=yaml has an extra level of grouping that seems unhelpful.

@greg-1-anderson
Copy link
Member

Use a @command annotation to give a command a precise name instead of using the default generated from the method name.

Return an AssociativeList datatype with a format of table (the default for AssociativeList) to get a key-value output listing. The constructor of AssociativeList should be given a simple key-value associative array of string => string items. If you want to return a string => array result (e.g. to preserve complex structures when the user selects --format=yaml), then create a new data type that extends AssociativeList and implements [RenderCellInterface](https://github.com/consolidation/output-formatters/blob/master/src/StructuredData/RenderCellInterface.php}. Then, on a cell-by-cell basis, you can convert from an array to whatever flat (string) representation is appropriate for tabular output.

@greg-1-anderson
Copy link
Member

greg-1-anderson commented Oct 4, 2016

Sorry, I should have read the code before I wrote all of that. :) The description above is for commands like Drush Status, where each row is a different key => value pair. For the sha command, returning a RowOfFields is appropriate.

Update: The shell-alias command is like Drush status, with the alias name serving as the key of each row. I pushed a commit that fixes the output formatter per the above explanation.

@weitzman
Copy link
Member Author

weitzman commented Oct 4, 2016

Thanks. added a few items to the checklist at top.

@greg-1-anderson
Copy link
Member

Hm, you are right -- there is an extra array wrapped around the data in AssociativeList. That's not right.

The output formatters project will need to expose some formatting options to get those other changes to work. Should be doable.

@greg-1-anderson
Copy link
Member

I fixed the extra wrapped array in --format=yaml in the upstream. composer update should fix this problem, but packagist isn't talking to me right now for some reason.

The table formatter already supports removing the dashes; just add @table-style compact to the command annotation. I pushed a commit to do this.

@greg-1-anderson
Copy link
Member

In the current version of Drush, shell-alias, core-status et. al. do not have headers at the top of the table.

@weitzman
Copy link
Member Author

weitzman commented Oct 5, 2016

Thats true. I was anticipating the need when we get to pml, for example.

@weitzman
Copy link
Member Author

weitzman commented Oct 5, 2016

I looked quickly and the output now has no headers nor a : delimiter between the two columns in the table. It looks unclear where each row cell starts and ends. I'm fine with borders everywhere like the screenshot at http://symfony.com/blog/new-in-symfony-2-7-advanced-table-layouts-for-console-commands.

@greg-1-anderson
Copy link
Member

Borders everywhere is easy to do; adding the : is hard at the moment, though, because Symfony doesn't support table column widths until version 3.1, and Drupal currently blocks us from using that.

pml and similar use RowsOfFields output, which by default has column headers. Of course, I'd recommend against porting pm-* and make, because I'd like to avoid touching the engines for now. If we did convert those, though, the output wouldn't be a problem.

@weitzman
Copy link
Member Author

weitzman commented Oct 5, 2016

OK thanks. For shell-alias, lets do borders everywhere and column names. Not urgent but I think thats a happy place to get to.

@greg-1-anderson
Copy link
Member

Don't run composer update on this branch until 8-to-master is merged, as the latest updates to support the stable release of the consolidation projects have already been done in that branch, but not here.

You can try @table-style default if you want to see borders everywhere output. The problem with this output (and most of the available options) is that Symfony doesn't support wordwrap in tables. We could potentially add that on as a layer above Symfony, maybe even support column widths prior to 3.1.

@greg-1-anderson
Copy link
Member

output-formatters will need some work to support column labels for AssociativeList.

@weitzman
Copy link
Member Author

weitzman commented Oct 5, 2016

I thought I saw that lack of wordwrap elsewhere. Thats really annoying IMO. A quick search does not show me other projects that have solved it like we have.

@greg-1-anderson
Copy link
Member

Yeah, it would be really beneficial to add the Drush wordwrap algorithm to the output-formatters table transformation class.

Merge #2373 to unblock this issue.

@weitzman
Copy link
Member Author

weitzman commented Oct 7, 2016

Starting simple - filed that feature request at consolidation/output-formatters#31

I also got confused about whether to use AssociativeList or RowsOfFields for this command. I think RowsOfFields is correct. Mainly, we need add column headers for the table to make sense to me.

@greg-1-anderson
Copy link
Member

Perhaps I should have named AssociativeList PropertyList instead. Perhaps I should still do that now, and make an empty class AssociativeList extends PropertyList to preserve backwards compatibility. consolidation/output-formatters#32

Whatever it is called, it is definitely the right data structure to use here. A property list is a collection of key : value pairs, with the keys displayed in the first column. By contrast, RowsOfFields is an array of records or rows, each of which are an array of fields (or, perhaps you could think of each record as a PropertyList). If we used RowsOfFields here, then we'd have to manually put the data inside an array (to make a RowOfFields with one row), and there would be no good way to get rid of it again when folks selected --format=yaml et. al.

Using AssociativeList / PropertyList here is no impediment to having column headers. We can add these in easily if we wish to do this. All it would take would be a little more metadata in the FormatterOptions that stipulate what the column headers should be labeled. consolidation/output-formatters#33

Finally, the table format always wants to try to convert the raw field / property into a "human readable" label by calling ucfirst() on every word. There should probably also be FormatterOptions metadata to preserve the raw value, so that the shell alias names do not go through this transformation. consolidation/output-formatters#34

@weitzman weitzman merged commit f123d42 into master Oct 15, 2016
@weitzman weitzman deleted the shell-alias branch October 15, 2016 00:14
gaelg pushed a commit to gaelg/drush that referenced this pull request Nov 7, 2016
@greg-1-anderson
Copy link
Member

Wordwrapping has been added to output-formatters

mikeker pushed a commit to mikeker/drush that referenced this pull request Aug 10, 2017
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.

2 participants