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

Refactor commands to share a common class #659

Open
di opened this issue Jun 11, 2020 · 6 comments
Open

Refactor commands to share a common class #659

di opened this issue Jun 11, 2020 · 6 comments
Labels
enhancement question Discussion/decision needed from maintainers

Comments

@di
Copy link
Member

di commented Jun 11, 2020

The work done to implement #381 and #511 has indicated that the lack of a common (internal) API for each of our sub-commands has made it challenging to implement flags like --verbose and --no-color across all commands.

#381 (comment) outlines a rough proposal for a TwineCommand which would allow sharing common patterns between commands:

  • twine-wide flags can be implemented in a single place
  • helpers for producing output
  • settings can live on the class instance and not be passed through multiple layers of function calls

This would help ensure that all commands behave in a cohesive manner. It also could lead to a more "short and sweet" implementation for #194.

Implementing this should not result in any changes to Twine's command-line API or behavior. It likely would change our unofficial, internal API that is used by some external projects

@sigmavirus24
Copy link
Member

@di I wish you'd take the time to discuss this with the folks working on the code. We have a problem and you've provided a solution without discussing it or if it actually meets the needs.

Many of the flags that you mention make perfect sense on the Settings object as we already have settings related to terminal output:

disable_progress_bar: bool = False,

It's used by all of our commands that we have (upload, register) and it's meant to be the central repository of common settings like this (just like the Repository instance handles the repository-related configuration work).

a TwineCommand base-class really just serves as busy-work for someone while deepening a rift between the zest.releaser folks and us. Those external projects aren't using a stable API, but that doesn't mean we should aggressively break things for them. I've worked hard to keep things stable until someone had the time to build the real API.

@di di changed the title Refactor commands to share a common class Proposal: Refactor commands to share a common class Jun 12, 2020
@di
Copy link
Member Author

di commented Jun 12, 2020

@sigmavirus24 I had hoped to have that discussion here. Is there somewhere else we should have discussed this first before creating an issue?

I had just intended to be a proposal, as far as I'm aware nobody has started work on anything related to this. To make that more clear I've added "Proposal" to the title.

I mostly just wanted to capture @VikramJayanthi17's ideas from #381 in a separate issue, so it were not lost.

Do we have a list of what third parties are using our internal API and how they are using it? If not, it'd probably be good to try and document that somewhere so contributors (and maintainers) have a clear idea of what can and cannot be changed.

@sigmavirus24
Copy link
Member

I had hoped to have that discussion here.

That was not obvious. Everything still in your original message (save the now edited title) speaks to "We're doing this. No discussion needed."

Do we have a list of what third parties are using our internal API and how they are using it?

Written down? No. In my head, there are at least 2 that I know of and probably others I don't. I think, frankly, the most important thing to document is how not to break those users of the undocumented, unsupported (but unfortunately widely used) API.

@bhrutledge bhrutledge added the question Discussion/decision needed from maintainers label Jun 14, 2020
@bhrutledge bhrutledge changed the title Proposal: Refactor commands to share a common class Refactor commands to share a common class Jun 14, 2020
@bhrutledge
Copy link
Contributor

Aside: I've been using the question label to indicate "discussion/decision need from maintainers". Applied here, which seems to make the "Proposal" redundant, so I removed it.

@di
Copy link
Member Author

di commented Jun 15, 2020

Written down? No. In my head, there are at least 2 that I know of and probably others I don't.

I'd like to document this, besides zest.releaser what other third parties do we know are using the undocumented/unsupported API?

@bhrutledge
Copy link
Contributor

For future reference, there's a long thread about reworking Settings in #649 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement question Discussion/decision needed from maintainers
Projects
None yet
Development

No branches or pull requests

3 participants