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

Support Acquia CLI #67

Merged
merged 6 commits into from Oct 24, 2021
Merged

Support Acquia CLI #67

merged 6 commits into from Oct 24, 2021

Conversation

danepowell
Copy link
Contributor

@danepowell danepowell commented Oct 18, 2021

Fix #66

Follows examples of #45 and #49

@ktomk
Copy link
Collaborator

ktomk commented Oct 18, 2021

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.

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
Copy link
Contributor Author

danepowell commented Oct 19, 2021

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
Copy link
Contributor Author

Done, thanks. This should be good to go.

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
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.

@danepowell
Copy link
Contributor Author

I adjusted the wording of the README

@ktomk
Copy link
Collaborator

ktomk commented Oct 21, 2021

Just seeing the list in the README isn't alphabetized fully, I guess because of console as first entry, but in the readme its composer.

And I could start the build now and it shows its red. Please let me know if I can be of help. I might also be able to take a closer look later.

@danepowell
Copy link
Contributor Author

I removed whitespace from the test fixture, I'm hoping that fixes tests. Otherwise I don't know what's going on.

@ktomk ktomk merged commit 60c3474 into bamarni:master Oct 24, 2021
@ktomk
Copy link
Collaborator

ktomk commented Oct 24, 2021

Thanks, looks good with the tests now.

@ktomk
Copy link
Collaborator

ktomk commented Oct 25, 2021

Released in 1.4.2.

@danepowell
Copy link
Contributor Author

Thanks for the quick response!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Autocompletion not working with Acquia CLI
2 participants