networkx / nx-parallel

A networkx backend that uses joblib to run graph algorithms in parallel.
BSD 3-Clause "New" or "Revised" License
34 stars 21 forks source link

Chore/style cleanup utility functions #80

Closed dPys closed 1 month ago

dPys commented 2 months ago

Summary

This PR focuses on stylistic improvements and cleanup of utility functions.

Goal

Ensure better package maintainability, scalability, and ease of contribution for new developers.

Code Style Adjustments

Utility Function Cleanup

Removal of Unused Imports/Code

@dpys

dPys commented 2 months ago

@Schefflera-Arboricola -- all type annotations have now been removed. If there are no other issues, then it might be helpful for us to get this merged sooner rather than later so that I can begin working on some of the other features that we discussed. WDYT?

dPys commented 2 months ago

I don't feel strongly about any of these comments, but these are some suggestions for choices during the style cleanup.

Thank you so much for the review! I've committed your formatting suggestions and agree with all of your comments.

dschult commented 2 months ago

What is the program that is creating the .history files? :)

dPys commented 2 months ago

I think we're getting bogged down here in linting land... :( Can we change the .gitignore file in a separate PR?

Yes, happy to revert the .gitignore to main :) We can address this on a separate PR as well once this PR gets over the finish line.

dPys commented 2 months ago

What is the program that is creating the .history files? :)

The Local History extension of VScode creates .history folders to store automatic backups of your files so you can revert to previous versions, similar to how PyCharm's Local History works.

For now, I can say that if the preference is to be more conservative with what goes into the .gitignore file, then it is no problem for me to just use my local .git/info/exclude file which doesn't get committed to the repo :) With that said, the current .gitignore is missing quite a few common entries (.DS_Store being one of them, as it applies to all MacOS users)

dPys commented 1 month ago

@MridulS -- it seems like removing the ruff dependency with that direct push to main is causing the style / format check to unconditionally fail for all PR's? Was that something that you were planning to revert?

Schefflera-Arboricola commented 1 month ago

@MridulS -- it seems like removing the ruff dependency with that direct push to main is causing the style / format check to unconditionally fail for all PR's? Was that something that you were planning to revert?

Can you try moving the update-get-info precommit hook at the end in .pre-commit-config.yaml file in this PR, because that hook internally runs a ruff command and @MridulS removed the ruff dependency. But, I think if we run the ruff hook before update-get-info hook then ruff would be installed in the pre-commit environment for the update-get-info hook to use. I think this should resolve the failing style test. Also, I did the same thing in the PR that @MridulS made but that PR is made from the main branch of nx-parallel so i think that's the reason the style test is passing for the push but not for the pull_request. (PR https://github.com/networkx/nx-parallel/pull/84)

for future refernce - CI error msg : _nx_parallel/script.sh: 5: ruff: not found