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

Rename chain to chain_success #33

Closed
pgaskin opened this issue May 25, 2020 · 6 comments · Fixed by #36
Closed

Rename chain to chain_success #33

pgaskin opened this issue May 25, 2020 · 6 comments · Fixed by #36
Assignees
Milestone

Comments

@pgaskin
Copy link
Owner

pgaskin commented May 25, 2020

In hindsight, chain was a bad choice of name due to the changes in #20. One potential way to fix this while keeping backwards-compatibility would be to rename it to chain_success, but continue accepting chain. I would probably remove chain completely sometime before v1.0.0, though.

See #32 (comment).

@pgaskin pgaskin changed the title Maybe rename chain to chain_success Rename chain to chain_success May 25, 2020
@pgaskin
Copy link
Owner Author

pgaskin commented May 25, 2020

I'd like some more opinions on this before I make a decision (about the option itself and about backwards-compatibility).

@NiLuJe
Copy link
Collaborator

NiLuJe commented May 25, 2020

I don't think many people are currently using this, and removing the action will very obviously generate a "config error" entry, which would make the deprecation fairly obvious and easy to fix for those few users caught unaware ;).

@pgaskin
Copy link
Owner Author

pgaskin commented May 25, 2020

I think we currently have a few hundred users of NM (from the GH releases, we probably have at least that many from your OCP packages), but it's growing relatively quickly compared to my other projects (I think this may become more popular than the patches).

Of course, as you said, only a fraction will be using the chain functionality, as most general-purpose user-oriented features are part of single options. I'm mainly concerned with backwards compatibility due to the fact that NM will a larger percentage of ordinary (with less technical knowledge) users using it than things like kobopatch.

I'd add install-time telemetry to this (and kobopatch, too) if I was sure there wouldn't be backlash against it (privacy concerns and all that). It'd be useful to know the number of active users and which NM versions they are on (at least for firmware versions, I have the stats from kfwproxy, which seem to represent the version distribution well in general).

@pgaskin
Copy link
Owner Author

pgaskin commented May 25, 2020

I think I'm going to rename it for v0.1.0, but emit a specific error for chain (which will eventually be removed).

@pgaskin pgaskin added this to the v0.1.0 milestone May 25, 2020
@pgaskin pgaskin removed the proposal label May 25, 2020
@pgaskin
Copy link
Owner Author

pgaskin commented May 25, 2020

While I'm at it, do you think I should also rename it to chain_ok and chain_failure to chain_err to make it shorter?

@NiLuJe
Copy link
Collaborator

NiLuJe commented May 25, 2020

I think I prefer success/failure (easier to grok, and readable as "chain on success"/"chain on failure") ;).

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

Successfully merging a pull request may close this issue.

2 participants