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

[discussion] new community rule: changes *must* have a real user #1566

Closed
maximbaz opened this issue Nov 2, 2018 · 13 comments
Closed

[discussion] new community rule: changes *must* have a real user #1566

maximbaz opened this issue Nov 2, 2018 · 13 comments

Comments

@maximbaz
Copy link
Contributor

maximbaz commented Nov 2, 2018

I've noticed that justification for some or other changes is being brought up again and again, so I'd like to propose to establish a simple community rule:


Any proposed change must be backed up by a real need from a real person


This would apply to a wide range of PRs and issues:

  1. new module
  2. new feature
  3. extend existing functionality
  4. make code more complicated/configurable/extensible

Before even reviewing the code, we should ask author the following questions:

  • is author going to actually use this module/feature?
  • does author know anyone specifically who will be using this module/feature?

If the answers to both questions are no, we reject and close the PR. If there's at least one yes, we don't necessarily agree to merge, but proceed with out usual workflow.

I'll make some examples, but please don't take offence if your PR is used as an example, I'm not trying to blame anyone, but simply to open up a discussion.

  • add new module: wanda_the_fish  #1447 new module: wanda the fish

    Is it actually used by anyone? If not, why increase the number of modules to maintain?

  • Default formats wtf. #1563 adding colors to default format

    Depends on the module. In sysdata I am personally using colors to indicate CPU load, so I don't have a problem with that. But for example in New module: MEGA sync #1458 MEGA module neither author of the module (me) nor any user of that module expressed the actual need for colors, so I prefer to refrain from complicating the PR with extra code that has no real use at the moment.

  • module: add background and border colors #1486 background and border colors

    Is there anyone who will actually use that if it was merged now?

  • Usbguard module #1376 "dashboard" view in usbguard

    Initially the module was very simple, showing only new devices that need approval. Later a whole "dashboard mode" was requested and implemented, where we can in addition see either all devices, approved or denied ones. I don't argue, it sounds super-cool, but again, does anyone actually use that? I know both the author and me have both simply disabled all that fancy functionality.

I'd like to repeat, I don't want to blame anyone and I certainly not saying that all the above is wasted effort, maybe these things are used, but the users never spoke up about using them.

I'm simply proposing to agree on what we merge and why we merge, specifically I want us to agree to validate the assumptions before implementing them. I don't agree with adding features "because we can", so I'd like the authors of open PRs to explicitly comment who will be using this new feature if it was merged right now.

@tobes
Copy link
Contributor

tobes commented Nov 2, 2018

One idea was to have new features etc added first as issues and then having the community vote on if it was wanted. I think this may have been @ultrabug

We can do this with PRs too. I'd be in favour of us all voting 👍 or 👎 on the opening comment of an issue/PR to show a preference. It creates less noise and may help influence things, or discover what is important to us. We need to maintain our inclusivity, so explaining why is important too.

@maximbaz I'd love it if you would make a PR adding Community Rules /doc/contributing.rst looks a good place. But maybe wait till we have had a chance to discuss it more here. I think the essence of your rule is perfect, I think the "from a real person" is redundant, if there is no person then there is no need. One community rule per PR is also a given, but we should also have as few as we can so, for me one total is plenty.

I'll also add that I'm far from perfect on the fun only features. How many real users of rainbow are there? Still I maintain This is the most pointless yet most exciting module you can imagine.

Can we all try to stick to the main topic in this issue; not get lost in discussing specific examples or assigning any blame.

@maximbaz
Copy link
Contributor Author

maximbaz commented Nov 2, 2018

  1. Voting

    I was also considering voting, but at the same time I'm not sure what to do when PRs have a real need from the author, but other contributors don't care about having this particular feature (not opposed and not supportive). Let's take external_script: show notification with full output on click, refresh only on button_refresh #1439 external_script: show notification with full output on click, refresh only on button_refresh for example, if I can't get a single 👍 from any of you because you don't use the workflow that requires output_full, does it mean I have to live with my own fork of this module?

  2. Document Community Rules

    Sure thing, will make a PR once this discussion is over and we agree on something!

  3. ... I think the "from a real person" is redundant ...

    With that phrase I wanted to highlight the difference between a thing that doesn't sound like a real need (let's say, if someone would send a PR that would every minute change text color in all py3status modules, for fun) and things that sound like very good ideas, like they could be used by someone, but in practice nobody confirmed that they actually will use it (just one example from usbguard, a full dashboard allows you to approve a device that you previously denied without having you to unplug and plug back that device — it sounds like a smart idea, but in practice every current usbguard user disabled that feature, so why was it implemented?).

@tobes
Copy link
Contributor

tobes commented Nov 2, 2018

  1. I think voting will never be perfect. I think with your example external_script: show notification with full output on click, refresh only on button_refresh #1439 I'd vote for the need but not for the implementation, because I'm for generic features that modules can all share. I think that PR also is suffering from getting so long that we get lost. stick to topic

@tobes
Copy link
Contributor

tobes commented Nov 9, 2018

@ultrabug @lasers @cyrinux and anyone else who wants to join in.

It would be good to have your opinions on this subject

@cyrinux
Copy link
Contributor

cyrinux commented Nov 9, 2018

Hi guys,

I'm agree with the statement "Any proposed change must be backed up by a real need from a real person".

This make also sense with ultrabug/tobes desire to have more small PR than few (more sick) big PR.

About modules, we can iterate through versions, quickly merge PRs that solve author's need, and if a different user discovers this module and needs a different need we can solve this in separate PR.

From my python learner point, another thing important is when PR grow to much, this become very tiresome. Don't say I'm not happy to learn with cool people, but I will be more motivated to work with several small PR/small goal which can be closed quickly, then if I need to breathe IRL, I can without thinking all the day that I must close my PR and I'm a prisoner ball

^^.

@tobes
Copy link
Contributor

tobes commented Nov 9, 2018

I will be more motivated to work with several small PR/small goal which can be closed quickly

Yeah, small PRs are good for all of us. Especially when it comes to learning to code. Also there are different levels of code quality that I expect. A first time contributor is not going to be encouraged into the open-source family if they are not given support, and they are also unlikely to do any harm.

As far as I'm concerned this is part of the purpose of this project. That is not to say that I don't take it seriously.

@lasers
Copy link
Contributor

lasers commented Nov 9, 2018

Replying to @maximbaz's comment...

Any proposed change must be backed up by a real need from a real person.

In that case, I should close all of my pull requests because I'm doing this for fun, not needs. I was interested to see what I can bring to py3status.

new module: wanda the fish

Users have different needs. I use this, but not with fortune. I have her swimming in the corner when I'm not debugging py3status. It does not take up much space and I like having a fish on my bar. Maybe in future, this give somebody an idea to improve this some more by adding different (non-fish) sprites.

I don't use xrandr_rotate, mega, do_not_disturb and yubikey. All this are yours. 🙂

adding colors to default format

Good idea. Less work for users.

for example in #1458 MEGA module neither author of the module (me) nor any user of that module expressed the actual need for colors, so I prefer to refrain from complicating the PR with extra code that has no real use at the moment.

MEGA is not real time. That's why I suggested colorized size. I feel the default format could be much better than almost static MEGA Synced. It'd be just MEGA 7.89 GiB where 7.89 GiB can be green, yellow, red. We can do thresholds strings now and after syncing, the number can change to something different. More dynamic and tad useful than Synced.

background and border colors

This exists for years so we're already years behind with making this possible. I believe we will have some users right away when they find out... as we already know we have a user with min_width so it's definitely something to add. This could bring in both new and former users.

"dashboard" view in usbguard

Well, this is interesting. The former code does not permit much flexibility and there were a small bug or two. Doing it right exposes (all) devices on the bar with individual indexes instead of using up buttons. Maybe great for laptops as you can point and click instead of middle click, left click, etc. Dashboard categories things (via filtering) making the bar much smaller. You essentially don't have to open USBGuard anymore. All of that can be removed and go back to what you like.

All codes just have its pros/cons and @Cyrinx asked me to help him with this. No more, no less.

I'd like to repeat, I don't want to blame anyone and I certainly not saying that all the above is wasted effort, maybe these things are used, but the users never spoke up about using them.

All of this wasted efforts is mine. I'm not sure how we can get users to speak up about using them. They just use them and they only tell us when there is an issue or they want to change something.

I want to know if there are users for wwan_status, screenshot, aws_bill, coin_balance, getjson, glpi icinga2, kdeconnector, ns_checker, pingdom, player_control, rss_aggregator, rt, transmission, and many many more.

I don't agree with adding features "because we can", so I'd like the authors of open PRs to explicitly comment who will be using this new feature if it was merged right now.

I'm trying to put everything through formatter instead of direct entry. All modules have its own issues when I went through them.. such as lack of thresholds, hardcoded colors, etc. It's really hard (and too too too too long at the rate we're going) to get everything up to date.


Replying to @tobes's comment...

One idea was to have new features etc added first as issues and then having the community vote on if it was wanted.

This supposedly already are in effect. I would say it didn't work. Nothing happened. Nobody voted or they did, but we can't tell if we're voting, thumbing up each others, or acknowledging received messages.

I'll also add that I'm far from perfect on the fun only features. How many real users of rainbow are there? Still I maintain This is the most pointless yet most exciting module you can imagine.

I'm all for module culling and/or consolidating.


Replying to @Cyrinx's comment...

desire to have more small PR than few (more sick) big PR.
About modules, we can iterate through versions

I like small PRs too. It's sometimes easier to redo module from scratch then you put in the backward compatible because it could be hard to iterate if the original code is not ideal in first place or are not optimized (ie post_config_hook).

quickly merge PRs that solve author's need

Even if it may not be ideal? Or that the module was not even ideal in first place?

When I said ideal, it could mean anything such as lack of color formatter so sure, yeah, we merge it, but we still hadn't allow formatter... and then when we allow it, apparently, how we got data was implemented in a way that it only get one thing (instead of everything).. so now we get everything this time and put all that in the formatter too. But now the new configs (from merged PRs) were not necessary and we can't make it work with the new formatter. This example is from systemd.

I try hard to avoid unnecessary placeholders and configs, but will always fail because I need to support old configs or old placeholders.

My solution is to fix everything in all modules right away before we get more PRs from users wanting to add this or this... and in the future, we could give them correct format... and focus on something different this time such as making i3status.py, cleaning up cli, core, dbus_listener, etc instead of modules, modules, modules. Half of the modules still have forced i3status-style colors.

@maximbaz
Copy link
Contributor Author

maximbaz commented Nov 9, 2018

In that case, I should close all of my pull requests

Why, not true! To make a few examples, I'm waiting for #1471 because it solves my problem of correctly synchronizing the state of dunst-py3status, and #1552 is a more generic alternative to #1545 (although I'm not sure if it needs to be this generic, or instead it should be something like #1266, which is simple and allows any helper). keep that discussion in #1552.

because I'm doing this for fun, not needs. I was interested to see what I can bring to py3status.

So my proposal here is to distinguish "for fun" and "for need". How about making a github.com/lasers/py3status-contrib repo and putting there all the fun modules? We will not commit to maintain them, so your and @tobes' and @ultrabug's job is easier, and yet we showcase what's possible, inspire people, and in case someone says "hey, I'm using that module from contrib, why is it not in core?" we instantly move it to core.

What do you think about it?

new module: wanda the fish

Users have different needs. I use this, but not with fortune.

My point here is: you use the fish — perfect! It's a valid argument for having it in core. Nobody uses fortunes? Fortunes are cool, but let's not merge "fortune" functionality in this repo until someone needs it. If we go with contrib approach (which we don't have to, it was just one idea), you can have "wanda the fish with fortunes" module there, inspiring people and showcasing possibilities.

I want to know if there are users for wwan_status, screenshot, aws_bill, coin_balance, getjson, glpi icinga2, kdeconnector, ns_checker, pingdom, player_control, rss_aggregator, rt, transmission, and many many more.

I agree! We need to find out this, somehow. Let's make a new issue and discuss possible approaches that we can take to achieve this?

I don't use xrandr_rotate, mega, do_not_disturb and yubikey. All this are yours.

I use all of these except xrandr_rotate. Valid point, how do we cleanup repo if authors of certain modules stop using them? Removing modules is harder than adding them, because we don't know who might be using them. Finding out which modules people use would be extremely helpful here.

mega colors

That's why I suggested colorized size.... More dynamic and tad useful than Synced.

This is a cool idea, as I told you before, but right now nobody will use this extra code, we will not even know if/when it breaks. That's why I'm making this proposal, let's not add code until we know that someone is using it.

background and border colors

I believe we will have some users right away when they find out...

How about we post this as a Github issue first, mark it with tag Discussion and ask people: making background and border colors configurable, will you use this and how? As soon as there are confirmations, we make the PR.

On the other hand, this is a bit special case because it's not a module, but a core feature. So I don't totally object this particular PR, it's kinda the same as #1548, where i3status supports something and py3status breaks it.

"dashboard" view in usbguard

The former code does not permit much flexibility and there were a small bug or two. Doing it right exposes (all) devices on the bar with individual indexes instead of using up buttons. Maybe great for laptops as you can point and click instead of middle click, left click, etc. Dashboard categories things (via filtering) making the bar much smaller. You essentially don't have to open USBGuard anymore. All of that can be removed and go back to what you like.

Again, not arguing about awesomeness of this solution, it is awesome 🙂 My point is, the PR became huge with functionality that nobody is using, and the PR is still open. If there were a couple of bugs, I think we should have just fixed those two bugs and allowed @cyrinux to merge the PR. If or when there comes a user request for more functionality, we implement it.

I'm trying to put everything through formatter instead of direct entry.

This is fine, if you see feature X implemented directly, you ask the author to implement this same feature via formatter. All I want is to avoid expanding the scope at the same time and implementing five other features that author isn't planning to use anyway 😉

All modules have its own issues when I went through them.. such as lack of thresholds, hardcoded colors, etc.

That might be not a problem, if nobody needs thresholds, why add them now? But maybe I'm taking this to the extreme, we can also agree on having some "basic set of features" that every module must have, if we say that "thresholds are a basic feature" then for sure, don't merge a new module until it implements thresholds. But this needs to be a team decision.

I try hard to avoid unnecessary placeholders and configs, but will always fail because I need to support old configs or old placeholders.

To some extend, backwards compatibility is important, but you can't allow it to poison your life. If you see that there's too much work needed for backwards compatibility, make an issue and say: "hey @ultrabug, I want to cleanup this and that, and it will break backwards compatibility here and there, can we please make a plan for 4.0"? Then list all the modules that you want to cleanup, and I'm sure the solution will be found.

@tobes
Copy link
Contributor

tobes commented Nov 9, 2018

we can also agree on having some "basic set of features" that every module must have,

Similar to format for modules that we did in the past :)

if we say that "thresholds are a basic feature" then for sure, don't merge a new module until it implements thresholds. But this needs to be a team decision.

I'd also add, imho, that we want to have as much done in core to keep modules clean and easy to create and maintain

@ultrabug
Copy link
Owner

Ok, to help me get into this conversation I've tried to summarize an extension of the py3status philosophy that is meant to frame our decisions.

Let me try to apply it to this discussion:

Any proposed change must be backed up by a real need from a real person

This ends up in the it's not because you can that you should part imho

So I agree with the fact that it should not be added, but I'm not sure we should make it a rule because if the proposal respects efficient and simple defaults AND one feature / idea at a time while serving the author's needs, that's fine

Now let me take some of your examples:

#1447 new module: wanda the fish

I think it's fun and harmless, that's why I accepted it just like I accepted rainbow.

One issue that I raised was about it's possible strong dependency on fortunes because modules are responsible for user information and interactions would mean that to use this fun module users would have to install something. Since it was not the case, I was ok with the contribution even if no user was requesting it.

#1563 adding colors to default format

This one related to rely on i3status color scheme so I will always be against this kind of additions. As a i3 user myself (heh) I dislike modules messing with MY color scheme.

#1486 background and border colors

If this relates to rely on the i3bar protocol then we should have it in core because core is responsible for user experience

#1376 "dashboard" view in usbguard

This one, while good willed, is a disaster and goes against one feature / idea at a time and good enough is good enough and it's not because you can that you should

The intention was to get efficient and simple defaults with strong user experience but the result is that it's still not benefiting anyone because it's too big & complex now.

How about making a github.com/lasers/py3status-contrib repo and putting there all the fun modules?

I'd like to kindly remind everyone that this project is about Open Source Software and I fully support its aspects and this is one of them. So if you were to feel like you need to do this and would ask for our official support, I would gladly add it as an external and optional dependency.

That being said, I would find it deceptive that we couldn't get a running and simple set of Zen "rules" to allow a faster iteration in a single project. That's why we have two talented collaborators and we could have more (hint)!

I hope this helps. Now I will go to create 3.15 and 4.0 issues to discuss what will be in them.

@maximbaz
Copy link
Contributor Author

I think we agree, I'm happy to close this issue in favor of #1582.

That's why we have two talented collaborators and we could have more (hint)!

To be fair this community is welcoming and I feel empowered even in the current state (my questions are answered, PRs are reviewed and my feedback is respected), but I'd be happy to officially change my badge from "Contributor" to "Collaborator" 😉

@ultrabug
Copy link
Owner

Adding this for reference:

maximbaz: about breaking changes, module consolidation and module rewrites, I wanted you to have it in zen, are you in general happy to see PRs such as new updates that will delete three existing modules or not

as long as we don't have the config rewriter, no I'm not keen on this
when we have it, we can do things like this easily

@ultrabug
Copy link
Owner

ultrabug commented Dec 2, 2018

I think we agree, I'm happy to close this issue in favor of #1582.

That's your issue to close, I don't want to force close it.

I'd be happy to officially change my badge from "Contributor" to "Collaborator" wink

Let's have a chat on IRC then mate

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

No branches or pull requests

5 participants