search

open-forest-observatory / geograypher

Multiview Semantic Reasoning with Geospatial Data
BSD 3-Clause "New" or "Revised" License
10 stars 4 forks source link

Integrated Black and Isort for code formatting #78

Closed asidhu0 closed 4 months ago

asidhu0 commented 4 months ago

This pull request uses black and isort to automate code formatting after a push or pull. It is configured with GitHub Actions, so if unformatted code gets pushed to the repository there will be automatic formatting changes that get pushed immediately after.

asidhu0 commented 4 months ago

@russelldj Since both isort and black can push formatting changes to the repository, having both workflows run at the same time results in concurrency issues. For example, isort will run first and push its changes to the repository. While this is happening, black will already have checked out the code but will not be able to push its changes due to diverging branches. To work around this, I first let isort run and then black runs after the completion of isort.

Another solution would be to combine the two workflows into one file but I prefer having a sequential approach to enforce modularity.

Let me know what you think and if this a good approach!

russelldj commented 4 months ago

This sounds like a good approach. Would this potentially create two commits or just one? One would be preferable, but two is fine.

asidhu0 commented 4 months ago

It does show up as two commits. This is because we have two workflows running and each workflow pushes reformatted code to the branch individually. The only way for there to be one commit is if we have a single workflow.

russelldj commented 4 months ago

That's completely fine, I was just curious. Just to confirm, it will only add a commit if changes are made, right?

Also, it sounds like this is coming along nicely. Can we get it merged soon and make additional modifications later?

asidhu0 commented 4 months ago

Yes, it will only add a commit if the workflow makes changes. Specifically, if the Black workflow makes a change and the Isort workflow does not, then only the Black workflow will make a commit, and so there will be one commit (vice versa). Two commits are only made when both workflows make changes.

I just have to rebase the commits and then it'll be ready to merge!

russelldj commented 4 months ago

Nice! Do you think this is ready to go now? I'm a bit confused why there are so many commits authors by both of us?

russelldj commented 4 months ago

It looks like the "push changes" part is failing. Any idea what's going on?

asidhu0 commented 4 months ago

@russelldj The reason why the black workflow was not being triggered by the isort workflow was because a workflow can only be triggered if the workflow file is on the default branch, which is main. So currently black is not being triggered but after the merge, the isort workflow will begin to trigger the black workflow since both workflow files will be on the default branch.

To fix the error when it was failing on the "push changes" part, I ensured the actions/checkout checked out the code from the correct branch.

Let me know if the commit history looks fine or if I need to make any other modifications. I pulled in the commits from main before rebasing my own commits so the commits look fairly messy as they are right now. I am not sure if there is a way I can change the commits or organize them in a more organized way. Other than that this branch is ready to be merged!

russelldj commented 4 months ago

Nicely done!