search

psf / black

The uncompromising Python code formatter
https://black.readthedocs.io/en/stable/
MIT License
38.62k stars 2.43k forks source link

Improving the contributing experience #2238

Open ichard26 opened 3 years ago

ichard26 commented 3 years ago

If you came here because you were asked to provide feedback:

Hi there! First of all I'd like to personally thank you for your contribution(s) to Black! This project is only possible by your contributions :black_heart:. Two, one of our top priorities is making it a good experience to contribute here. We would love to hear from you about your feedback and suggestions (context why can be found further below).

If you can please comment with your feedback and suggestions. Any sort of constructive feedback is welcomed. It can be as general as "the contributing documentation is lacking" or as specific as "on X environment, the test suite breaks", although in general the more specific and actionable the better. Bonus points if it's about the contributing documentation since I personally (as in me @ichard26) have been working on improving them lately.

Thank once again!

Context & why

Black is a popular project and probably gets a significant amount of potential contribut(ors|ions). Black is also a more mature and older project with a lot of bugs to squash so we need a fair of constant development to keep the project running smooth. One of my top concerns is that we are wasting / losing a lot of potential contributions because a poor contributing experience (fits in the general maintainability goal I've been striving for). It's not like the maintainers of this project can handle everything.

A few reasons why we might be losing potential contributions:

But the thing is that I'm a maintainer so a lot of things that used to annoy me don't anymore since I got used to them. Asking contributors (especially first-time contributors!) for feedback and suggestions is better. Sooo... hence this issue.

fyi to fellow maintainers: I plan to use this issue as a place to collect feedback and suggestions as I ping contributors during the PR process

felix-hilden commented 3 years ago

Nice! I agree for the most part. Some thoughts:

"How do I write a test?"

I assume this and other questions like it are mainly for the specific structure of Black tests, i.e. writing the code twice to a Python file and so on. Otherwise I'd consider it out of scope.

Requirements / environment / installation

Kind of agreed, although I don't have any experience with Pipenv outside of Black. What do you think are the most pressing issues? I'm fine with the installation, although of course having the ability to use any virtual environment would be nice. Is the locking specifically what you want, or would a requirements file or another entry in extras_require, like pip install -e .[dev] be enough?

Responsiveness

Yeah this is too bad, but I can definitely see why it happens. The automation is top notch though, and documenting processes and expectations help tremendously.

Support for contributors / chat

This was discussed a bit already in #2176. I see why having a separate chat platform for maintainers is useful for a project this big. And the same for contributors would surely be nice too. What I don't like about that is that information can be missing from the issue tracker, and it seems that Richard you agreed that there is a some way to go with transparency. Something that would maybe satisfy transparency, ease of access but a certain type of exclusivity would be a chat platform with some sort of rights or channel management. Discussions between exclusive groups would be public but safe from spam. For example Discord works wonders, but anything like that would also be cool. How does the IRC channel work?

ichard26 commented 3 years ago

Update:

For example Discord works wonders

Hahaha, funny you mention that since right now we're in the process of figuring out a Discord based communication area. Our IRC platform (Freenode) is falling apart right now so that's no longer an option. IRC is sorta like a Discord server but with only one channel (and it's text only). Permission management is also way less powerful and the feature set is way weaker. IRC is really old (created in 1988) but it was a good (open and simple; no walled gardens!) platform for real-time communication. Anyway, it's looking like either we're securing an official spot in the Python Discord server or we might just create our own server. The benefit of going under the wing of Python Discord is that we don't need to do our own moderation. OTOH, administrating our own server would give us a lot of flexibility. I personally prefer making our own, but take that opinion with a grain of salt since I haven't had to moderate a large community before :upside_down_face:

Responsiveness

Yeah it's honestly just more of a "changing what is prioritized" game than really anything at this point. I'll continue to document as much I can but eventually I should probably set aside some of my F/OSS time for just PR review or issue triage (as difficult or frustrating they can be).

RE: "How do I write a test?"

I was more thinking about documenting the (limited) testing infrastructure we've built over the years. Creating a simple format test case is as simple of creating a file and adding it to a list, but to a brand new contributor, they might not know that and create an unnecessarily complicated test.

RE: Requirements / environment / installation

I find it frustrating how a pipenv install --dev doesn't install all of the dependencies needed to do all development operations. The test suite has its own independent test_requirements.txt file. I understand why it exists (speed) but I wish a full dev environment specified by Pipfile.lock is good enough.

The thing is that we're basically supporting two kinds of development commands. Tox based ones (e.g. tox -e py3) and not-Tox based ones (e.g. sphinx-build docs/ docs/_build/html -E. I personally prefer the direct commands because they're much faster and provide a great deal of control (the environment management provided by Tox isn't necessary for me). But the Tox commands are stupidly easy to use and that's what we want for less experienced contributors. Overall, more consistency regardless of what kind you choose would be great.

felix-hilden commented 3 years ago

We're in the process of figuring out a Discord based communication area.

Awesome! 😄 I'm of course in the favor of a dedicated server. The Python server is quite big, and a community this large would be better off in a dedicated server in my opinion. That way new users would be welcomed appropriately instead of having to search around for Black things. No harm in setting up a channel on the Python server though, that could be a good place to catch some people!

When it comes to managing such communities.. I think it's quite easy if some channels and rights are restricted heavily. In terms of banning the worst users though, I don't know. Never had to deal with communities this large either. But I've set up a server for a library of my own and it has worked quite well. BTW, don't know if you remember, but you went out of your way to give some information about the isort 5 release related to Black for us so: thanks again! You're the best. Let me know if I can help at all with that!

Eventually I should probably set aside some of my F/OSS time for just PR review or issue triage

I admire your dedication 😅

RE2: "How do I write a test?"

Yup 👍

RE2: Requirements / environment / installation

Okay, I'm reading that you'd still like to keep the Pipenv stuff and a simpler setup doesn't work for Black, so I'll let people that are smarter than me comment on that!

Development commands.

Personally I like tox -e docs and the like. What kind of consistency? Do you mean simply implementing the commands? That could very well be an actionable issue to create from this.

temeddix commented 3 years ago

To leave some of my thoughts:

I'm not really used to making pull requests, and this was my first time on #2229. However I felt people contributing to this repository was very kind and guided me well. I just wish this goes on.

The way I fixed this bug is simple. Personally I didn't needed any documentation. I only followed the exception traceback message and dived into my Anaconda site-packages folder where my Black PIP module was installed. I was available to make PIP module installed on my PC to work. After that I made the same code changes to the fork of this repository and made a pull request.

My environment is Windows 10 64-bit with Anaconda. Hope this helps :)

P.S. I was curious about when the next release with my pull-request will be out and hoped to find out in some way, but I did not yet.

doublevcodes commented 3 years ago

As I understand it, you now have a dedicated channel in the Python Discord server (#black-formatter) under the Open Source Projects category. This was made specifically for Black but may involve future partnerships in PyDis' end. Is there still going to be a dedicated server or does this suffice?

cooperlees commented 3 years ago

As I understand it, you now have a dedicated channel in the Python Discord server (#black-formatter) under the Open Source Projects category. This was made specifically for Black but may involve future partnerships in PyDis' end. Is there still going to be a dedicated server or does this suffice?

We're trailing sharing with Python Discord server, and, all going well, this should suffice.

ichard26 commented 3 years ago

For future me reference: check pre-commit's behaviour on Windows regarding our local Black hook. Ref: https://discord.com/channels/267624335836053506/846434317021741086/847259516641738773

HassanAbouelela commented 3 years ago

Here's my feedback after getting my first PR merged (#2259). For context: I'm on windows.

Firstly, this project has had one of the smoothest experiences I've had with OSS projects, though some things can still be improved.

Technical Problems

I've run into a few problems while setting up the project (mostly because I'm on windows), but they have been for the most part resolved. The only thing remaining is the pre-commit problems in ichard's comment above. The exact problem and possible solutions are linked in that comment, so I won't go into details here for brevity.

I would like to thank the maintainers for being quick to figure out the problems and help fix them as they were raised.

Experience Interacting With Maintainers

I don't really have much to say here. I was assigned pretty quickly, and my PR was reviewed and rereviewed in a very timely manner. All in all, everything was done within a week, which is very quick.

Other Comments

I would like to give my thoughts on the suggestions made so far.

I found the testing part to be fairly alright, though the monolithic files made figuring out things a bit difficult at first (this is true for the entire project, not just tests). The main thing I think should be clarified is the data folder, and its purpose.

For installation, I'm with you about having all dependencies installed from pipenv. Having to install 3 different sets for things most developers will need is not an ideal experience. I get the point about speed, but the contributing experience takes a serious hit. I found this especially true when I was downgrading my environment to test a feature in an earlier python version, and basically had to repeat the entire setup again.

One thing that doesn't seem to have been considered yet are pipenv scripts. Pipenv natively supports scripts to run CLI commands (if you're familiar with JS development, think npm/yarn scripts). Perhaps you could have a script to run pip install for the docs, tests, and extras. It's still not ideal, but it makes it one extra step instead of 2 or 3.

Pipenv scripts could also be considered instead of tox (one important limitation to note is that pipenv doesn't support multiple commands in one script like tox does).

HassanAbouelela commented 3 years ago

On a different note from my other feedback: if you're considering some form of mentorship program/event, that may be something you can work on with the Python Discord staff. I know I'd personally be happy to help with something like that.

Jackenmen commented 3 years ago

So after trying to contribute I've noticed some small issues with local testing:

I ignored both of these since I knew neither of these problems were caused by me but seeing a test failure or being unable to commit due to pre-commit check (which the person might not necessarily know how to disable) might drive some contributors away from actually finishing their contribution and PRing it since they don't know what they've done wrong.

gaborbernat commented 3 years ago

All good from my side, very straight forward. pipenv lock initially failed on one of my machines, but luckily had another one that passed.

MarcoGorelli commented 3 years ago

Just to add a few thoughts from my experience with the Jupyter PR:

Overall, the experience was really nice and I learned a lot from the reviews - thanks!

ichard26 commented 3 years ago

OK, sigh, it's been a while since I've last looked at this mini-project. Gosh do I have too many projects for Black. Anyway, I just want to take some time to respond to the lovely feedback already collected here.

I found it a bit surprising that and after pipenv install --dev I didn't have pytest installed in the environment

Yep yep yep, I already know of this and am currently working on fixing this.

the mypy pre-commit hook would sometimes throw errors when running on staged files but not when running on all files. I used no-verify to make those commits and then ran pre-commit run --all-files to check it all worked

Mypy and pre-commit's ability to limit the hooks to only run on the changed files don't always play nicely. I doubt psf/black can do anything about this to resolve this ... other then maybe writing a "list of random issues you may run into + solutions" document :)

All good from my side, very straight forward. pipenv lock initially failed on one of my machines, but luckily had another one that passed.

Yea, pipenv has been a pain for me as well actually. I've been trying to add the test dependencies to the lockfile and there has been a lot of buggy behaviour. I'm weary of this sort of tools in general, but there has been some work on poetry so perhaps I'll take a look into moving Black to that instead.

[...] might drive some contributors away from actually finishing their contribution and PRing it since they don't know what they've done wrong.

Well noted! Thanks for the perspective!

On a different note from my other feedback: if you're considering some form of mentorship program/event, that may be something you can work on with the Python Discord staff. I know I'd personally be happy to help with something like that.

Weellll, let's just say we don't have the maintainer resources to do that ... like at all. I'm swamped with my own projects and the other maintainers just don't have much free time and/or have other OSS duties to tend to. But in general, I'm happy to organize stuff with PyDis and I expect the others to feel the same.

The main thing I think should be clarified is the data folder, and its purpose.

Ah yeah I 100% agree the contributing docs need a lot of love right now. That is one of projects so one day I'll get them done.

Pipenv natively supports scripts to run CLI commands

Since I've started this project how ever many months ago, I've started a new personal open source project related to my mypyc work. In that project trying to use more modern tooling than I'm used to so I ended up using Nox. Now I'm pretty happy with it and am way happier to support tox / nox tooling. I personally prefer Nox, but we can figure out the details later.

I can't promise much (I'm human after-all) but I will try to commit to at least figuring out a general plan and writing the details up so y'all can help out. Thank you so much for your time and feedback :slightly_smiling_face:

ichard26 commented 3 years ago

Alright, some high-level planning.

Documentation

Developer experience

jalaziz commented 3 years ago

Just chiming in.

I had an overall smooth experience. Nothing stood as out of the ordinary from a contribution perspective.

The only awkwardness I ran into was with the PR checklist. In particular, since I was fixing the binary build, it wasn't clear how I would write a "test" for the fix. To be far, this is not unique to black. It's always easier to add new tests to a project when a similar test exists and can be copied and adopted.

I also panicked a bit in trying to figure out how I would build the binaries to test my fix. Luckily, I realized that the GH actions were working on my fork and figured out I could create a release on my fork. I would imagine that might be problematic for folks who are less comfortable with github.

Shivansh-007 commented 2 years ago

Here is my feedback after getting my first PR merged (#2526).

Firstly, I would like to thank all the wonderful maintainers of black 🖤 (especially Richard, Felix and Jelle) who have helped me throughout the process and made it super smooth, though there was a bit of ups and downs in my PR 😉.

I actually found the code really well written and helpful, it is well documented, not very hacky, and has good naming, which really helps when you are new to the code.

I think the only thing is the code is not well distributed among folders/files, especially __init__.py (where my current contribution was focused). It has two large components grouped together, the formatting handler (ipynb, files, strings, etc.) and CLI app, this is more of personal preference but I find it a little difficult to grasp the two components if I see them a bit mixed up 😄. I think the two deserve a separate file, keeping __init__.py just about ~10 lines to call the CLI app.

One more thing I had like to raise is merge conflicts in CHANGES.md 😔, every time someone pushes a change to main/new version is released, there are conflicts which though are to the point, get a bit annoying. A solution for this is to introduce something like town-crier, though I would like to have a custom implementation according to our needs (I could out with this, as I am already writing one for discord-modmail), this would not only reduce the merge-conflicts but make the experience with writing changelogs much better 😄.

Removing pipenv was a good change 👍🏻.

Other than that, it was wonderful.

ewjoachim commented 2 years ago

Hey there 👋 I offered to give some feedback after contributing in #2814 and was redirected here. Before my feedback, I was reading the comments above and I wanted to chime in:

One more thing I had like to raise is merge conflicts in CHANGES.md 😔

Yep, that. In my projects, I've shifted to a different approach, where I removed the changelog in the code, but I use release-drafter to have the changelog in the GitHub releases based on the merged PR titles since the last tag, as well as sphinx-github-changelog (disclaimer: I'm the author of that tool) to have the changelog in the sphinx doc. That said, it shouldn't be hard to have GitHub actions make a PR to CHANGES.md whenever a release is published, so that people wouldn't have to bother with the changelog, but it would not add too much strain on the maintainers who cut the releases either and still end up in the released code. That said, towncrier is nice too (it's just that I always love a process where releasing the lib doesn't require commits/PRs/...)

So regarding my feedback:

First and foremost, thank you a lot for your reactivity, and jumping to help and offering advice.

The hardest part on my PR was writing the test. I'm not saying it's bad, but I think it could be improved in several areas:

If you're interested, I'll be glad to contribute one or more of those feedback items.

JelleZijlstra commented 2 years ago

Thanks a lot @ewjoachim for the detailed feedback! We have #2766 for discussing an improved changelog workflow. We'd also love to make the unit tests better.

ichard26 commented 2 years ago

Minor update: I've opened GH-2857 switching out tox for nox (the tl;dr is that it's nicer to work with) while also rewriting the contributing getting started document. If y'all want to try 'em out / read my docs and give feedback that'd be great.

Also yes pipenv is no longer a thing here so no more weird locking issues :slightly_smiling_face:

gaborbernat commented 2 years ago

Minor update: I've opened GH-2857 switching out tox for nox (the tl;dr is that it's nicer to work with)

You might want to wait for the soon to be released version 4 of tox... By the way what parts do you consider nicer? I would love to hear specifics rather than generics, so they're more addressable. (and yes will have soon python file config too, for everyone wanting imperative rather than declarative configs)

tomjelinek commented 2 years ago

I contributed a one-line fix #2905. There was no update to documentation and tests, so I cannot talk about that. I had no issues figuring out how to set up and run tests, the documentation is clear and easy to find and follow in this matter. Responsiveness to the PR was the top notch, the PR got merged in 4 hours, which is superb!

Thanks to everyone working on this awesome project!

yoerg commented 2 years ago

Fixing #2904 was actually quite pleasant, so good job! I had to work on Windows for this, but everything worked as documented. The only thing I had to look up on SO was how to run just a single unit test – never did this with tox before.

ichard26 commented 2 years ago

Ah that's unfortunate to hear, will try to remember to tack on that kind of information in our contributing docs :+1: