Skip to content

Conversation

BDadmehr0
Copy link

@BDadmehr0 BDadmehr0 commented Mar 29, 2025

@jbampton

AUTHORS file look like mruby/AUTHORS

Copy link
Member

@jbampton jbampton left a comment

Choose a reason for hiding this comment

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

You should remove all the unneeded code comments

Copy link
Member

@jbampton jbampton left a comment

Choose a reason for hiding this comment

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

Seems that some entries are duplicated

Perhaps a groupby / merge is missing for commit author ??

Copy link
Member

@jbampton jbampton left a comment

Choose a reason for hiding this comment

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

If you run pre-commit locally it will help format your files to be compliant.

Otherwise you can run "black" on your Python files.

And make sure there is no trailing whitespace in the Authors file.

You can see these issues in the pre-commit failure workflow.

https://black.readthedocs.io/en/stable/

run black and remove comments
list with user's usernames
@BDadmehr0
Copy link
Author

fixed @jbampton

Copy link
Member

@jbampton jbampton left a comment

Choose a reason for hiding this comment

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

We have to decide what to do with the new bandit issues shown in the pre-commit failure workflow

https://github.com/PyCQA/bandit

https://bandit.readthedocs.io/en/latest/

@jbampton
Copy link
Member

It seems the commit totals are not 100% correct.

@AmirTallap would you like to review this PR ?

Is there an easier way to do this ?

- Using `shutil.which("git")`: Resolves **B607** (partial executable path)
- Checking for the existence of `git` and handling errors: Increases security and prevents crashes
- Error handling in `subprocess.check_output`: Addresses **B603** and follows better security practices
- Keeping `subprocess` usage only with safe paths and hardcoded arguments: Fully resolves **B603**, **B607**, and **B404**
Copy link
Member

@jbampton jbampton left a comment

Choose a reason for hiding this comment

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

We should not store the Python script in the repo root. We try to minimize the amount of files in the root.

There is a tools folder perhaps we should make a sub directory there called scripts so it would be tools/scripts.

Is there a better location ?

@AmirTallap
Copy link
Contributor

It seems the commit totals are not 100% correct.

@AmirTallap would you like to review this PR ?

Is there an easier way to do this ?

Checking!

@BDadmehr0
Copy link
Author

BDadmehr0 commented Mar 30, 2025

I fixed all the Bandit errors that were resolvable, but Bandit raises default security warnings for subprocess (B603 and B404), which don't have alternative solutions, so we need to silence them. @jbampton

b3d9a63

Bandit output

[main]  INFO    profile include tests: None
[main]  INFO    profile exclude tests: None
[main]  INFO    cli include tests: None    
[main]  INFO    cli exclude tests: None    
[main]  INFO    running on Python 3.13.2   
Run started:2025-03-30 08:22:17.869637 

Test results:
        No issues identified.

Code scanned:
        Total lines of code: 71        
        Total lines skipped (#nosec): 1

Run metrics:
        Total issues (by severity):    
                Undefined: 0
                Low: 0
                Medium: 0
                High: 0
        Total issues (by confidence):  
                Undefined: 0
                Low: 0
                Medium: 0
                High: 0
Files skipped (0):

@BDadmehr0 BDadmehr0 requested a review from jbampton March 30, 2025 19:55
@jbampton
Copy link
Member

Hey @BDadmehr0 thanks for all of this.

Pretty sure the commit totals are not correct for some contributors

@BDadmehr0
Copy link
Author

Hey @BDadmehr0 thanks for all of this.

Pretty sure the commit totals are not correct for some contributors

Hello, sorry for the delay.

I've improved my script, but I need to make sure the current script isn't working correctly. If you're referring to the file https://github.com/apache/sedona/pull/1895/files#diff-ab6af77435f58cc0c9d4c31dfe05656e45187cc7c7fc02aada401a7642125463, I should mention that this script needs to be run in a cloned environment of https://github.com/apache/sedona/ on the main branch that has been recently updated to correctly show the commits.

Please test this and let me know.

@BDadmehr0
Copy link
Author

BDadmehr0 commented Apr 14, 2025

Why do I want this? Because my improved script also doesn't fetch the latest commits in my cloned branch. @jbampton

> python tools/scripts/generate_authors.py                                                                                      ~/Github/sedona(AUTHORS✗)@archlinux
AUTHORS file generated successfully.

Top contributors:
 1.   424 Jia Yu @jiayu
 2.   368 Jia Yu 
 3.   167 John Bampton @jbampton
 4.    78 Kristin Cowalcijk 
 5.    57 Nilesh Gajwani 
 6.    56 Furqaan Khan @46216254/furqaankhan
 7.    37 jinxuan wu 
 8.    36 Furqaanahmed Khan @46216254/furqaankhan
 9.    35 zongsizhang 
10.    34 Kristin Cowalcijk @kontinuation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants