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

[Update] Breeze2-on-windows #20148

Merged
merged 1 commit into from
Jan 3, 2022

Conversation

edithturn
Copy link
Contributor

@edithturn edithturn commented Dec 8, 2021

According to this issue:
#19992 <- Already merged
Closes #19966

I am adding the documentation to install Breeze2 on windows, with "pipx"

BREEZE.rst Outdated
Comment on lines 274 to 282
.. code-block:: bash

pipx install -e dev\breeze

If you are using Linux/Bash (POSIX) on Windows, like Git Bash use:

.. code-block:: bash

pipx install -e dev/breeze
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forward slash works everywhere, including Powershell and cmd.exe, so we don’t need two code blocks here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does it :) ? Did they learn ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, they learned. / works for PowerShell, cmd, and Git Bash. Thank you I will fix it.

BREEZE.rst Outdated

.. code-block:: bash

pix ensurepath
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pix ensurepath
pipx ensurepath

BREEZE.rst Outdated
@@ -265,6 +265,40 @@ You should set up the autocomplete option automatically by running:

You get the auto-completion working when you re-enter the shell.

Breeze on Windows
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment - I think for now we should develop a "parallel documentation. I have not thought about it too much - but I believe for now - until we open Breeze2 for others, we should keep the docs in ./dev/breeze/doc.

I am not 100% sure what will be the best structure there. Should we have a separate "Breeze_install.md" ? Or everything in README.md or something else? Maybe as a 'fresh' user - you could come up with some proposal @edithturn ? . For sure we should not add it to the current Breeze docuementation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And we should also add docs for other platforms. Actually pipx is not Windows specific. If as @uranusjr says "forward slash" works on Windows. then we could provide universal installation instructions for any OS.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with this, BREEZE.rst documentation is already big. I don't know about the structure yet because I am not familiar with everything, but next week I came come with an idea. For now ./dev/breeze is good.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly all the documentation related to Breeze2 should be in a separate directory, right? If it is like that, I think this directory is good: ./dev/breeze/doc, but we should think about what things we will behave here, For now, I just know we have BREEZE.rst documentation. (Correct me if I am wrong)

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs would be better in ./dev/breeze (see the comment)

@edithturn
Copy link
Contributor Author

The docs would be better in ./dev/breeze (see the comment)
@potiuk I will make a copy of BREEZE.rst with the changes about Windows and I will add it here: ./dev/breeze/docs. Is that okay?

@potiuk
Copy link
Member

potiuk commented Dec 9, 2021

The docs would be better in ./dev/breeze (see the comment)
@potiuk I will make a copy of BREEZE.rst with the changes about Windows and I will add it here: ./dev/breeze/docs. Is that okay?

I think it woudl be better Just start "fresh" the BREEZE documention basicaly has everything but a kitchen sink, and I think it will be better outcome if we start from zero (and maybe move relevant start from Breeze.rst and decide later what to do when we switch.

Actually - I think a simple/standard approach is to have INSTALL.md in the "project" directory. And maybe that's a good starting point ("./dev/breeze/INSTALL.md"). Md is much easier/more lightweight format for documentation and seems to be better suited for that. Unless others have another opinion. @uranusjr WDYT?

@edithturn
Copy link
Contributor Author

I got it @potiuk, I also think .md is easier, and we should try to standardize the documentation too (.md or .rst for new docs) :)

@potiuk
Copy link
Member

potiuk commented Dec 9, 2021

.md it is

@edithturn
Copy link
Contributor Author

@potiuk let me know if this is good: https://github.com/apache/airflow/pull/20148/files
I added BREEZE.md on dev/breeze/doc, this is practically empty, it has just Installation on Windows.
What I am thinking to do is, review and validate all the existing documentation on BREEZE.rst and add on BREEZE.md (which is a lot).
Another thing, I could focus only on information that is not updated, I can go over the documentation and ask you section by section to see if that part of the documentation should be deleted or updated, so just after know that I could add it on BREEZE.md.
Let me know your thoughts.

@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Dec 13, 2021
@github-actions
Copy link

The PR is likely ready to be merged. No tests are needed as no important environment files, nor python files were modified by it. However, committers might decide that full test matrix is needed and add the 'full tests needed' label. Then you should rebase it to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

@potiuk
Copy link
Member

potiuk commented Dec 13, 2021

Some static-check erors - needs pre-commit re-run.

@edithturn
Copy link
Contributor Author

Yeah, I will add more things and I will fix it! Thank you:
I am using as a reference another PR: #19966

```
To update the installation use: --force
```bash
pipx install --force -e <Breeze Folder>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pipx install --force -e <Breeze Folder>
pipx install --force -e dev/breeze

@potiuk
Copy link
Member

potiuk commented Dec 28, 2021

I am afraid you will need to rebase the changes due to conflict @edithturn. We have a nice instructions on rebasing in https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#id15

@edithturn
Copy link
Contributor Author

This part entry: ([\W\s\n\t\r]|^)breeze([\W\s\n\t\r]|$) was added. I will reverse my code.

@edithturn edithturn force-pushed the update-doc-install-breeze-windows branch from d809239 to f23a03c Compare January 3, 2022 15:51
@potiuk
Copy link
Member

potiuk commented Jan 3, 2022

LGTM

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Jan 3, 2022
@github-actions
Copy link

github-actions bot commented Jan 3, 2022

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@potiuk potiuk merged commit 9db894a into apache:main Jan 3, 2022
@potiuk
Copy link
Member

potiuk commented Jan 3, 2022

🎉 🎉 🎉 🎉 🎉 🎉 🎉

@edithturn
Copy link
Contributor Author

wow, I will cry with this, I am really happy to be in Apache Airflow. This was just documentation but in all these processes I learned a lot, thank you @potiuk :')

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:dev-tools full tests needed We need to run full set of tests for this PR to merge okay to merge It's ok to merge this PR as it does not require more tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Breeze: Make Breeze works for Windows seemleasly
3 participants