8000 Use argparse in scripts/format.py by adsharma · Pull Request #17360 · duckdb/duckdb · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Use argparse in scripts/format.py #17360

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 9, 2025
Merged

Use argparse in scripts/format.py #17360

merged 1 commit into from
May 9, 2025

Conversation

adsharma
Copy link
Contributor
@adsharma adsharma commented May 4, 2025

argparse is a widely used library and provides robust argument handling functionality.

@duckdb-draftbot duckdb-draftbot marked this pull request as draft May 4, 2025 17:11
@adsharma adsharma marked this pull request as ready for review May 4, 2025 17:11
@adsharma
Copy link
Contributor Author
adsharma commented May 5, 2025

The proposed invocation in duckdb/extension-ci-tools#174 doesn't work the same after argparse.

python3 scripts/format.py --all --check --directories src test

Translates to:

(Pdb) p rest
['test']
(Pdb) p args.revision
'src'

Then there is this code https://github.com/duckdb/duckdb/blob/main/scripts/format.py#L224-L226 which interprets args.revision as a directory and checks only the first level of src.

So only the test directory and parts of src directory get formatted.

My preference is to remove os.path.isdir(revision) code and remove the positional argument. Revision would have to be explicitly specified as --revision main.

@duckdb-draftbot duckdb-draftbot marked this pull request as draft May 5, 2025 01:35
@adsharma adsharma marked this pull request as ready for review May 5, 2025 01:35
@adsharma
Copy link
Contributor Author
adsharma commented May 5, 2025

Tested via:

python3 scripts/format.py --revision upstream/main
python3 scripts/format.py --directory src/include
python3 scripts/format.py src/include/duckdb.h
python3 scripts/format.py --all
make format-check
make format-fix

@duckdb-draftbot duckdb-draftbot marked this pull request as draft May 5, 2025 01:58
@adsharma adsharma marked this pull request as ready for review May 5, 2025 04:18
Mytherin
Mytherin reviewed May 5, 2025
View reviewed changes
Copy link
Collaborator
@Mytherin Mytherin left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Looks good - nice to move this to argparse. I think we can avoid the change in argument syntax, see my comment below:

Tishj
Tishj reviewed May 5, 2025
View reviewed changes
@duckdb-draftbot duckdb-draftbot marked this pull request as draft May 5, 2025 15:48
@adsharma adsharma marked this pull request as ready for review May 5, 2025 15:48
@duckdb-draftbot duckdb-draftbot marked this pull request as draft May 5, 2025 16:08
@adsharma adsharma marked this pull request as ready for review May 5, 2025 16:08
@adsharma
Copy link
Contributor Author
adsharma commented May 5, 2025

The failing check is due to a image pull problem from docker hub per co-pilot. Please rerun the failing check if you have permissions.

@duckdb-draftbot duckdb-draftbot marked this pull request as draft May 8, 2025 19:14
@adsharma adsharma marked this pull request as ready for review May 8, 2025 19:14
@Mytherin
Copy link
Collaborator
Mytherin commented May 8, 2025

Thanks! Looks great

@Mytherin Mytherin merged commit 6548e91 into duckdb:main May 9, 2025
51 checks passed
@adsharma adsharma deleted the arg_parse_only branch May 13, 2025 15:43
krlmlr added a commit to duckdb/duckdb-r that referenced this pull request May 18, 2025
@krlmlr
vendor: Update vendored sources to duckdb/duckdb@d741e1e
77caf69 
Add missing commas (duckdb/duckdb#17424)
Use argparse in scripts/format.py (duckdb/duckdb#17360)
krlmlr added a commit to duckdb/duckdb-r that referenced this pull request May 18, 2025
@krlmlr
vendor: Update vendored sources to duckdb/duckdb@d741e1e
faf5a46 
Add missing commas (duckdb/duckdb#17424)
Use argparse in scripts/format.py (duckdb/duckdb#17360)
krlmlr added a commit to duckdb/duckdb-r that referenced this pull request May 19, 2025
@krlmlr
vendor: Update vendored sources to duckdb/duckdb@d741e1e
05aa13a 
Add missing commas (duckdb/duckdb#17424)
Use argparse in scripts/format.py (duckdb/duckdb#17360)
krlmlr added a commit to duckdb/duckdb-r that referenced this pull request May 19, 2025
@krlmlr
vendor: Update vendored sources to duckdb/duckdb@d741e1e
3ed00b0 
Add missing commas (duckdb/duckdb#17424)
Use argparse in scripts/format.py (duckdb/duckdb#17360)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mytherin Mytherin Mytherin left review comments

@Tishj Tishj Tishj left review comments

Assignees
No one assigned
Labels
Ready To Merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@adsharma @Mytherin @Tishj
0