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

Language lists don't show error on missing arguments #27

Closed
schlessera opened this issue Jan 25, 2018 · 11 comments
Closed

Language lists don't show error on missing arguments #27

schlessera opened this issue Jan 25, 2018 · 11 comments
Assignees

Comments

@schlessera
Copy link
Member

I was just playing around with the new language command, but I couldn't get the list to work on a WP 4.7.9 install:

image 2018-01-25 at 8 18 39 am

I had the same problem with themes as well.

@schlessera
Copy link
Member Author

To give a bit of context, this site has never dealt with any localization, so the site's languages is still set to the standard english.

The problem might just be some value not even being initialized, and therefore the list returning empty. I'll investigate.

@schlessera
Copy link
Member Author

Okay, the problem is actually that it would need either one or more plugin names or the --all flag. If you provide neither, it shows an empty list. It should in that case throw an error.

@schlessera schlessera changed the title Installed languages not shown in list Language lists don't show error on missing arguments Jan 25, 2018
@gitlost
Copy link
Contributor

gitlost commented Jan 25, 2018

Yes I spent quite a bit of time last night trying to write tests for it and failing. I'm thinking that #21 should probably be reverted until these things are ironed out.

@schlessera
Copy link
Member Author

I'm already working on it. Will have a PR soon.

@gitlost
Copy link
Contributor

gitlost commented Jan 25, 2018

I don't think us both working on the same part of the codebase will work.

#21 just isn't mature or stable enough for now. I think it should be reverted and not be part of 1.5.0.

Edit: And even if we do get it to work, it's going to consume a lot of what time each of us has to work on other stuff for 1.5.0.

@schlessera
Copy link
Member Author

Okay, you're leading 1.5.0, so it's your call. I thought the command would already be usable (and useful), albeit not perfect. So, either we revert now, or as an alternative, we do the major fixes, release the command as it can already be used, gather feedback, and push a 1.5.1 to bring it closer to perfection.

@gitlost
Copy link
Contributor

gitlost commented Jan 25, 2018

The problem for me is that it changes the existing stable behaviour of the language-command. It's not a simple addition or non-perfect enhancement. It's a major refactoring.

So I vote revert.

@gitlost
Copy link
Contributor

gitlost commented Jan 25, 2018

Am removing the bug label as it's now WIP.

@swissspidy
Copy link
Member

swissspidy commented Jan 25, 2018

I think I copied this logic from wp plugin activate. Running that command without an argument doesn't throw an error either:

$ wp plugin activate
Success: Plugin already activated.

@schlessera
Copy link
Member Author

You're right, @swissspidy : wp-cli/entity-command#136

@swissspidy
Copy link
Member

I guess we can re-use most of the logic from https://github.com/wp-cli/extension-command/pull/83/files

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

No branches or pull requests

3 participants