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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix wrong default yes/no value in management commands #254

Merged

Conversation

abdullahkady
Copy link
Contributor

@abdullahkady abdullahkady commented Mar 9, 2020

This is pretty straight forward, a small typo that actually made me question my sanity for a couple of runs 馃榿
The default was shown as Y while the actual logical default implemented is N

@safwanrahman
Copy link
Collaborator

@abdullahkady How you are assuming that the default is Y?
#215 (comment)

@abdullahkady
Copy link
Contributor Author

abdullahkady commented Mar 9, 2020

I'm not assuming, the existing implementation defaults to No, but rather shows the default in the terminal as Y, so I just edited it to show the correct default (which is No)

I just read the issue you provided, I'm not sure if I understand correctly, but it seems that you don't think that Y/n means default to 'Y', and y/N defaults to 'N'. It's a convention as discussed here and here.
I believe if you don't want to follow it, it would be sensible that both letters will be small/capital (but in such case, the default should -imo- not be accepted at all)?

@safwanrahman
Copy link
Collaborator

@abdullahkady You are assuming "Y" is default by seeing the "Y" as uppercase. But there are no standard about that the default one should be uppercased.

@abdullahkady
Copy link
Contributor Author

@abdullahkady You are assuming "Y" is default by seeing the "Y" as uppercase. But there are no standard about that the default one should be uppercased.

I already linked 2 answers discussing it, you can see how popular cmd tools use it (such as apt when used in something that requires a confirmation such as autoremove, or for instance since I'm currently running it eslint)

In the end: whatever you think, it's your decision anyway.
But indeed there is a convention, which is capital letter means the default, if you don't plan on following it, at least don't follow an anti-convention? (aka what's the point of having a capital letter in the choice? it makes no sense).

@safwanrahman
Copy link
Collaborator

safwanrahman commented Mar 9, 2020

@abdullahkady I understand that it is mostly used. But it is not a convention. You can make the letter as small case and add a message like the '{}' indexes? [y/n] (default n) that would make sense actually.

Moreover, having a forum answer maybe counted as opinion, not reference.

@andreasnuesslein
Copy link
Contributor

andreasnuesslein commented Mar 10, 2020

@abdullahkady I've forked https://github.com/django-elasticsearch/django-elasticsearch-dsl and already added a few new features (it's on PyPI as django_elasticsearch_ng for now). I would gladly accept such a pull request because obviously you're correct - I don't see how this is debatable ^^

@alexgarel
Copy link
Collaborator

@safwanrahman I will kindly add my opinion.

If you see [Y/n] in a question of a unix CLI without any other indication, most people will assume Y is the default.

I think this really is enough of a problem to fix it by either:

  • display [y/N] if no is the default
  • explicitly display which is the default

but leaving it as is, is misleading for most of the users, hence a real problem.

@safwanrahman
Copy link
Collaborator

@alexgarel Thanks for your comment. I have mentioed about explicitly showing the default in #254 (comment) .

@andreasnuesslein Forking the repository without any strong reason does not make sense. It is just reinventing the wheel without any reason. Making the community devided for this small package will not bring any good things to the community.

@abdullahkady
Copy link
Contributor Author

@andreasnuesslein Thanks! but I'm not really a fan of just creating forks unless it's a major-non-compatible change because it divides the community for no reason.

@safwanrahman You can go ahead and implement the default is n if you'd like, I just see this conversation as a very one-sided conversation/opinion, so it doesn't really matter.

@safwanrahman
Copy link
Collaborator

@abdullahkady I am sorry if it seems like one sided conversation/opinion. As you have suggested earlier, making both smaller and mentioning the default explicitly make sense. As you have opened the PR (Thanks for that), it would be great if you can change it and push a new commit. I will be happy to merge it.

@andreasnuesslein
Copy link
Contributor

If this package actually picks up pace again I'll gladly merge back. However there haven't been any responses from both developers in a month or so. It's nice @safwanrahman, that you are back on this but it really feels like sabricot dropped out completely and you are also a bit too swamped to handle all the PRs coming in on your own.
I did invite @safwanrahman and @sabricot to http://github.com/django-elasticsearch in an effort to create a bigger team that would be able to maintain this but I hadn't heard back in weeks - ergo now the push for a fork

@safwanrahman
Copy link
Collaborator

@andreasnuesslein I think it is wise and norm to email the maintainers before creating a new organization and inviting them. Without any context, I have got a invitation to django-elasticsearch and declined because I did not like the approach. As far as I have checked, I did not get any email or any IRC message from you where you have proposed any organization or anything.

@andreasnuesslein
Copy link
Contributor

@safwanrahman kinda funny hearing about "the norm" from the guy who will not accept the actual norm of uppercasing the default on a CLI yes/no choice. ;)
And yes it might not have it's own PEP but I'm pretty certain 99.9% of the tools I use or have ever used stick to that concept.

However I'll give you that I could've been more verbose in my contact attempts. (I did not expect an answer to be honest.)

I do require a quick development pace with this package at the moment though so I'm wondering if you and or @sabricot would be willing to accept more maintainers to this project?

@alexgarel
Copy link
Collaborator

Hi @safwanrahman I just want to point that @andreasnuesslein is right on the point about the upper case argument, meaning default choice.

I hope, you will be able to understand each other for the benefits of most users.

@safwanrahman
Copy link
Collaborator

As @alexgarel is agree with the changes, I am ok to merge it. Thanks @abdullahkady for the change request.

@safwanrahman safwanrahman merged commit 7e6b12a into django-es:master Mar 12, 2020
@safwanrahman
Copy link
Collaborator

@andreasnuesslein I think you should appreciate the effort of the creator of this package and the current maintainer of this package. Your attitude seems to be disrespectful to both of them and also maybe make them depressed to contribute to open source.

Regarding maintainers acceptance, I have only collaboration access so I can not give anyone any access. Personally, I am open to more collaborators and contributors like @alexgarel and I have given him access on Read The Docs to publish our documentations. But the way you have approached, it is not wise enough to welcome anyone for collaboration.

I wish best of luck with your fork as you want a faster development face. Forking is easy and maintaining only own needs is easier than maintaining a general purpose package that can run in most of the systems.

@andreasnuesslein
Copy link
Contributor

It's interesting that you think I don't appreciate the effort - quite the contrary: I think the project is good enough that it should be allowed to grow.

However I think the fact that the only person able to actually add more maintainers/collaborators/developers has not been active for over a year is quite stifling to the project and in my opinion totally warrants a fork.

You may have noticed that when I forked I of course invited you and Sabricot to join and also added you and a few others to the authors list - credit where credit is due. https://github.com/django-elasticsearch/django-elasticsearch/blob/6837408964136ba054c51af681521d963fd20bfd/pyproject.toml

My goal was to enliven the project which I don't think is possible in this setting here (sabricot being the only one with the relevant rights). So, to whom it may concern: The fork is totally supposed to be for a broad audience and I'd love to add some maintainers.

@sabricot
Copy link
Member

I can add contributor or move the project somewhere else.
Tell me what you think is the best ?

@safwanrahman
Copy link
Collaborator

safwanrahman commented Mar 12, 2020

@andreasnuesslein If you really wanted to help the maintainers, you had other option as well. You could ask the people to create a separate github organization and move it to there. As I said earlier, you have sent me the invitation without any context or anything. So I did not like the intention and the way you have handeled it.

@andreasnuesslein
Copy link
Contributor

Hey @sabricot 馃憢

I can add contributor or move the project somewhere else.
Tell me what you think is the best ?

Obviously that's up to you, but I think you should grant @safwanrahman the rights to add people, even though I don't like him personally. But a) he's been the active developer over the last few months or so, b) he knows the other contributors and c) says he's open to adding more developers then. Or you add some people yourself as well if you have capable people in mind.
I'm not sure but I think you'd have to transfer the project to an organization to elevate his rights as I think it's not possible with a personal repo.

@sabricot
Copy link
Member

Ok, so @safwanrahman are you interested to take the lead on a new repo ?

@andreasnuesslein in that case you will still continue to maintain your fork ? or there is a world where you could both work together (maybe you know other interested people?, @barseghyanartur ?) to maintain the project ? (if you are interested of course).

@andreasnuesslein
Copy link
Contributor

Ahoy @sabricot ,

I actually never said I couldn't work on this with @safwanrahman (and others) together - it sounded more like it's the other way around.

Concerning the fork: Did you happen to have the chance to browse through my commits ( @ https://github.com/django-elasticsearch/django-elasticsearch ) ? Do you like the direction I'm taking there? If not and if my commits are not what you are looking for and therefore would not be merged anyways, the question whether I'd stick with the fork seems moot. In particular I think a better and more sophisticated auto discovery of the fields is paramount - stuff like this: https://github.com/django-elasticsearch/django-elasticsearch/commit/59126ef25a6bf56bb318f1df0ec0a6c3a5b17638 and https://github.com/django-elasticsearch/django-elasticsearch/commit/32abc3fe289280c2837b6fd4e3c1bb533de46e1f .

So yea - I could imagine working on this project - but maybe it would be good to have a quick outlook on the future then and see if expectations align.

PS: should we open a new ticket on the following debate? I feel we hijacked this issue long enough.

@barseghyanartur
Copy link
Contributor

@sabricot:

I don't think I will have enough time to contribute a lot, but I could review things and vote on decisions.

As of the original issue itself, I think we should be using defaults of Django in what concerns management commands and that's either Y or y (case doesn't matter).

@safwanrahman
Copy link
Collaborator

@sabricot I am good to take the lead of this project. I can commit some of my time to maintain this project.

@andreasnuesslein
Copy link
Contributor

( recent event, kind of underlining my point https://www.theregister.co.uk/2020/03/26/corejs_maintainer_jailed_code_release/ )

@prebenkeeeb prebenkeeeb mentioned this pull request Apr 28, 2020
@andreasnuesslein
Copy link
Contributor

@sabricot just a quick update, due to recent events: I still think you should put this into a github organization and add at least @safwanrahman as an "owner" or whatever the term is.

I could see myself creating PRs then based on the changes/updates I made to my fork and removing the fork afterwards, assuming @safwanrahman would be willing to merge them.

@safwanrahman
Copy link
Collaborator

safwanrahman commented May 28, 2020

Thanks @andreasnuesslein for your interest to push the changes upstream from your fork and maintain the upstream only. I will be happy to review and merge the patches that you will make.

@barseghyanartur
Copy link
Contributor

barseghyanartur commented May 28, 2020

+1 for the idea of moving this into organisation.

@alexgarel
Copy link
Collaborator

yes please @sabricot this seems like a good move, if you could take some minutes of your time to do it.

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

Successfully merging this pull request may close these issues.

None yet

6 participants