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

Improve output readability and download efficiency #145

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

hacksysteam
Copy link
Contributor

huangqinjin and others added 6 commits September 16, 2024 20:41
* Added colors to output messages to highlight important information.
* Switched from multiprocessing to ThreadPoolExecutor for downloads.
* Added tree like structure to --print-deps, --print-reverse-deps and --print-selection.
* Merged mstorsjo#140
* Merged mstorsjo#144
@hacksysteam
Copy link
Contributor Author

hacksysteam commented Sep 25, 2024

To check the color output you can check: https://github.com/mstorsjo/msvc-wine/actions/runs/11022479747/job/30611873227?pr=145

you should also check the output from --print-deps, --print-reverse-deps and --print-selection. it looks nice. Feedback is welcome.

Also the download efficiency increased after using ThreadPool.

Downloaded 2.9 GB in total in 20.11 seconds

@mstorsjo
Copy link
Owner

Sorry, but this changes way too much at once to be practically reviewable.

As you do essentially three different changes, I'd suggest splitting them up in three separate commits. If they're independent and don't clobber each other, they can go in separate PRs in parallel, otherwise they can go in separate commits within the same PR, or just do one of them in a PR, then post the next one once that is merged.

I also see that in addition to all other changes, this change intermixedly changes the whole script to use ' for quotes rather than ". Don't mix such changes with functional changes. If there's a reason to change style that can be agreed upon, then it has to be done in a separate commit, separate from all functional changes.

@hacksysteam
Copy link
Contributor Author

Hi @mstorsjo thanks for the comments.

As you can see that this PR has multiple commits. Two merged PRs and regarding ' and " I normalized the code to use " as this is what being used in most of the places in the code.

Let me know how you want me to proceed and I'll update the PR accordingly.

@mstorsjo
Copy link
Owner

Hi @mstorsjo thanks for the comments.

As you can see that this PR has multiple commits. Two merged PRs and regarding ' and " I normalized the code to use " as this is what being used in most of the places in the code.

Yes, I know your PR contains multiple commits, for the other PRs. But your own commit on top consists of 3 different functional changes, plus the fact that you’re normalizing the quote style for strings. Don’t normalize anything, keep untouched code as it was. Split your 3 separate functional changes into 3 separate commits.

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.

3 participants