Support Acquia CLI #67

With the opportunity to change the list, please put it on top, by the character < a > it is starting with (C case-insensitive collation sort). I'm aware that this is not the case for recent edits, but the original change-set suggest that.

Then #45 suggests that tests needs to be extended and perhaps some read-me changes. Please compare as well with #49 .

I'm fine if you amend and force push for the PR, but there is no requirement to do so.

Thanks for your efforts.

@ktomk

Copy link

Collaborator

ktomk commented Oct 18, 2021

And please amend each commit with a message that reflects your understanding of the change. That would be helpful.

And if then - separated by a single empty line - a reference to the original issue and maybe on another line to the PR - would be golden. This helps to keep track of changes over time. Short identifiers like < hash > + number is fine.

danepowell added 2 commits

October 19, 2021 12:20

@danepowell


 Add Acquia CLI (acli) to default tools

f4e9708

bamarni#66
bamarni#67

@danepowell


 Update README

84334ed

Add Acquia CLI to list of supported tools, and reword sections to make it clear that only tools named console or on the list of default tools are supported out of the box
bamarni#67
bamarni#66

@danepowell danepowell force-pushed the acli branch from 7652c05 to 84334ed Compare

October 19, 2021 19:28

@danepowell

Copy link

Contributor Author

danepowell commented Oct 19, 2021 •

edited

Loading

I made the changes you suggested and also updated the README to hopefully avoid further confusion for users and contributors in my position.

With the opportunity to change the list, please put it on top, by the character < a > it is starting with

I actually think it should be second on the list since console is the canonical entrypoint, and everything else on the list is secondary. I placed it first among the secondary entrypoints. We should probably have a follow-up PR to alphabetize that list, and also alphabetize and add links to the support tools list in the README.

@ktomk

Copy link

Collaborator

ktomk commented Oct 20, 2021

I actually think it should be second on the list since console is the canonical entrypoint, and everything else on the list is secondary.

Well spotted, very good thinking.

We should probably have a follow-up PR to alphabetize that list, and also alphabetize and add links to the support tools list in the README.

If you already have something of that prepared, you can also add another commit to this PR. Right now the acli link looks a bit alone in that list.

@danepowell


 Alphabetize all the things

5c18bac

@danepowell danepowell force-pushed the acli branch from aa727ea to 5c18bac Compare

October 20, 2021 16:35

@danepowell

Copy link

Contributor Author

danepowell commented Oct 20, 2021

Done, thanks. This should be good to go.

ktomk

ktomk requested changes

Oct 21, 2021

View reviewed changes

Copy link

Collaborator

@ktomk ktomk left a comment

Choose a reason for hiding this comment

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

Thanks for taking care with the read-me. I'd love to see the change a bit shifted a bit towards the previous wording, especially for the point of what is supported, as your changes (naturally) put focus on the use-case of adding a utility to that list, but I know this is not the only use-case and I got the impression it has too much focus.

The best Idea I've come up with is to give the hint about how to add to that list after the list.

README.md Outdated Show resolved Hide resolved

@ktomk

Copy link

Collaborator

ktomk commented Oct 21, 2021

The Github CI build also was stale so it didn't properly run. I've updated master, just in case for the next update, if you refresh on top of master, it should show recovered. Sorry I didn't see this earlier.