potiuk commented on PR #23762: URL: https://github.com/apache/airflow/pull/23762#issuecomment-1130592634
> A few thoughts: > > 1. I think it'd make sense to mention Breeze in [CONTRIBUTORS_QUICK_START.rst](https://github.com/apache/airflow/blob/bc6ad899adce71fd39ee01206c169312fe86fe4c/CONTRIBUTORS_QUICK_START.rst). Maybe just a high level overview and a link to [BREEZE.rst](https://github.com/apache/airflow/blob/bc6ad899adce71fd39ee01206c169312fe86fe4c/BREEZE.rst). Perhaps in the "Note to Starters" or "Local machine development" sections? Good point. Done. > 2. My concern would be how maintainable will these guides be? If something common to each of the guides changes, they're not very DRY, and the update will have to be made in each sub-guide. Maybe common parts like "Forking and cloning Project", "Setting up Breeze", "Installing airflow in the local virtual environment airflow-env with breeze.", "Using Breeze", "Testing", etc could be included in the "main" [CONTRIBUTORS_QUICK_START.rst](https://github.com/apache/airflow/blob/bc6ad899adce71fd39ee01206c169312fe86fe4c/CONTRIBUTORS_QUICK_START.rst). Perhaps there could be a link at the end of each sub-guide that returns to `CONTRIBUTORS_QUICK_START.rst` to finish the guide with these sections. Yeah. I think there was a bit too much duplication now when I looked at it. Should be much better now - I actually left jus the vscode/pycharm/gitpod/codespaces specific parts. This makes a bit more "go here", and then "click here", but with the amount of duplication we had it made indeed little sense. > 3. Nitpick, but in [CONTRIBUTORS_QUICK_START.rst](https://github.com/apache/airflow/blob/bc6ad899adce71fd39ee01206c169312fe86fe4c/CONTRIBUTORS_QUICK_START.rst), maybe it should say "Contributor's Quick Start" or "Contributor's Quick Start Guide" instead of "Contributor's Quick Guide". Ah yeah - did not see that :) > 4. Another nitpick: IMO [CONTRIBUTORS_QUICK_START_VSC.rst](https://github.com/apache/airflow/blob/bc6ad899adce71fd39ee01206c169312fe86fe4c/CONTRIBUTORS_QUICK_START_VSC.rst) should be named `CONTRIBUTORS_QUICK_START_VSCODE.rst`. surely > 5. Yet another nitpick: Each of the sub-guides starts with "Setup Airflow with Breeze" and "Forking and cloning Project" as a header. Maybe these headers should be removed; I'm not sure there should be a header for Breeze here because it's explained later in the guides, and there's no alternative to Breeze given. Does that make sense? Similarly, maybe the "Forking and cloning Project" header should be removed because it's described in step 1 and there's no alternative. > I reworked it quite a bit now in general. I think it will be much nicer. > Overall I think this is much better! Let me know what you think. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
