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.

Sign up for GitHub

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

Jump to bottom

Make --version a non-global option #143

Closed
wants to merge 2 commits into from
Closed

Make --version a non-global option #143

wants to merge 2 commits into from

Conversation

mkniewallner
Copy link
Member

Resolves #142.

@mkniewallner mkniewallner force-pushed the make-version-non-global-option branch 2 times, most recently from 9fe1cee to faba585 Compare May 28, 2022 10:23
mkniewallner
mkniewallner commented May 28, 2022
View reviewed changes
cleo/commands/list_command.py
self._io,
self.application,
namespace=self.argument("namespace"),
definition=self.definition,
Copy link
Member Author

Choose a reason for hiding this comment

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

Since application only has the default definition, I ended up passing definition as an extra argument, to retrieve it in _describe_application.
Otherwise, the command would not display --version option.

If there's a better way to handle that, I would gladly update that.

@mkniewallner mkniewallner marked this pull request as ready for review May 28, 2022 10:53
@mkniewallner mkniewallner force-pushed the make-version-non-global-option branch from faba585 to acddb23 Compare June 4, 2022 08:48
@Secrus
Copy link
Member

Secrus commented Aug 30, 2022

I would prefer for the --version to be available only on the application level. I don't see why it should work with list or help commands, since app list --version suggests that we want a version of list not of the app.

@mkniewallner
Copy link
Member Author

I would prefer for the --version to be available only on the application level. I don't see why it should work with list or help commands, since app list --version suggests that we want a version of list not of the app.

I share the same feeling, so I'm glad to read that. I'll rework the PR in that sense.

@Secrus Secrus mentioned this pull request Sep 4, 2022
Closed
@Secrus Secrus added this to the 1.0 milestone Sep 12, 2022
@Secrus
Copy link
Member

Secrus commented Oct 2, 2022

@mkniewallner I think I am gonna delay this feature to the 1.1 release. Took a look into the code and in the current state, this is a non-trivial issue to add an app-level only option. I plan on refactoring and improving the parser past-1.0 and with what I have in mind, this should be much easier then.

@Secrus Secrus modified the milestones: 1.0, 1.1 Oct 2, 2022
@mkniewallner
Copy link
Member Author

@mkniewallner I think I am gonna delay this feature to the 1.1 release. Took a look into the code and in the current state, this is a non-trivial issue to add an app-level only option. I plan on refactoring and improving the parser past-1.0 and with what I have in mind, this should be much easier then.

Makes sense. I didn't think it would require heavy changes when starting this PR, so entirely re-thinking the approach sounds better.

@Secrus Secrus modified the milestones: 2.1 , Future Jan 22, 2023
@Secrus Secrus closed this Sep 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Future
Development

Successfully merging this pull request may close these issues.

Make --version a non-global option?
2 participants
@mkniewallner @Secrus