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

Auto-format code #143

Closed
gschivley opened this issue Feb 16, 2022 · 4 comments · Fixed by #673
Closed

Auto-format code #143

gschivley opened this issue Feb 16, 2022 · 4 comments · Fixed by #673
Labels
enhancement New feature or request
Milestone

Comments

@gschivley
Copy link
Contributor

The GenX codebase has lots of lines that are much longer than 80/92 characters. Many projects (python, julia, etc) use auto formatters to ensure that code is always formatted in a consistent way. For example, PowerSystems.jl has a github action that checks all pull requests for formatting using JuliaFormatter.jl. I do the same with Black for PRs to PowerGenome. The advantages of auto formatting code are that 1) lines don't get too long and 2) everything in the codebase has the same visual style. Respecting line length makes it easier to view two files side-by-side. A uniform visual style makes it easier to read the code without forcing everyone to learn/follow a set of code guidelines.

@cfe316
Copy link
Collaborator

cfe316 commented Feb 17, 2022

I think this would be a great idea. My worry about the actual implementation would be that it would make git blame harder across the jump - or is there a flag to ignore certain commits?

I would love to see this implemented, perhaps just after bringing the codebase "back together", like if/when we merge back main and Dev before a next minor release. Otherwise I would imagine it would make things more difficult to merge two branches together.

As a related point, at least some of the long lines are long because they have complex interior expressions, not simply because they're, say, a long list. These should probably be rewritten or functionalized, not just formatted.

@gschivley
Copy link
Contributor Author

My worry about the actual implementation would be that it would make git blame harder across the jump - or is there a flag to ignore certain commits?

Looks like there is a way to do this (at least locally). It doesn't work on GitHub but there is an active request for the feature.

I would love to see this implemented, perhaps just after bringing the codebase "back together", like if/when we merge back main and Dev before a next minor release. Otherwise I would imagine it would make things more difficult to merge two branches together.

Agree that it's best done with or after a merge/PR.

As a related point, at least some of the long lines are long because they have complex interior expressions, not simply because they're, say, a long list. These should probably be rewritten or functionalized, not just formatted.

I don't disagree.

For anyone who isn't sure why it helps to auto-format the code, one advantage is that it changes this view
image

to this

image

Because the formatted code conforms to maximum line lengths (the default is 92) I can easily have side-by-side code windows. It also removes all personal choice about if statements should look like this

        if v println("Pre-removal: ", names(ModifiedDataNormalized)) end

or this

        if v
            println("Pre-removal: ", names(ModifiedDataNormalized))
        end

The benefit is that code looks the same everywhere without having to convince everyone to adopt the same style.

@cfe316 cfe316 added the enhancement New feature or request label Jun 3, 2022
@lbonaldo lbonaldo added this to the v0.4 milestone Jan 8, 2024
@sambuddhac sambuddhac linked a pull request Apr 1, 2024 that will close this issue
8 tasks
@sambuddhac
Copy link
Collaborator

PR #673 addresses this.

@sambuddhac
Copy link
Collaborator

Addressed on release branch

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

Successfully merging a pull request may close this issue.

4 participants