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

Add gdformat config file for excluding folders. #286

Merged
merged 2 commits into from
Mar 9, 2024

Conversation

chrisl8
Copy link
Contributor

@chrisl8 chrisl8 commented Mar 5, 2024

This is an initial stab at fixing Issue #250

Some decisions I made that could go differently:

  • I chose to use the gdlint config file for gdformat, instead of creating a separate one for gdformat.
  • I copied over several functions rather than attempting to reference existing functions or build common libraries.

This is sort of an "easiest possible path" solution. Feel free to suggest better alternatives, but this appears to work for me.

@Scony
Copy link
Owner

Scony commented Mar 6, 2024

Looks promising.

I've been thinking about the config file for a while and IMO using gdlintrc is not a proper way to go - it will be misleading and a change to use smth like gdformatrc would break compatibility in the future. Moreover, I'be been thinking about gdformatrc and I think we don't need it. IMO the best way to proceed would be by adding a new argument like --excluded-directories. It should be fairly straightforward to change the current PR to achieve that.

What do you think?

@chrisl8
Copy link
Contributor Author

chrisl8 commented Mar 7, 2024

The benefits of a config file that come to mind are

  • Easy to add a lot of entries to and maintain them
  • No need to remember to add them every time you run the command
    • It is handy to be able to fire off gdformat -c . at any time without needing to add exclusions.
  • Simplified and consistent github action settings
  • No confusion with other developers about what files to ignore, since the settings are in a file in the repository.

Basically a formatting tool operates similarly to a linting tool, in that I hope that other developers and myself can easily use it frequently without much fuss.

Also Godot has the added situation of expecting us to commit "addons" into the repo in the addons folder which we generally don't want to lint or format.

Finally, if all we intend to do is add a command line argument, at last in Linux, this is pretty easily done already:
find . -name '*.gd' -not -path "./addons/*" -print0 | xargs -0 gdformat --check

But it is nice that running gdlint is so much simpler:
gdlint .

and with a config file gdformat can also be just:
gdformat -c .

Prettier has a config file to ignore files: https://prettier.io/docs/en/ignore.html
Black also seems able to be configured via a file as well: https://black.readthedocs.io/en/stable/usage_and_configuration/the_basics.html#configuration-via-a-file

Those are my thoughts anyway and why I was wanting a config file for gdformat, a tool that I really love and hope to see become a standard for GDScript.

@chrisl8 chrisl8 force-pushed the gdformat-use-gdlint-exclude-folders branch from a68c57c to ab3fed7 Compare March 7, 2024 04:31
@Scony
Copy link
Owner

Scony commented Mar 7, 2024

Ok, that makes sense. Let's just rename to gdformatrc then

@chrisl8 chrisl8 changed the title Have gdformat read the gdlint config file for excluded folders. Add gdformat config file for excluding folders. Mar 8, 2024
@chrisl8 chrisl8 force-pushed the gdformat-use-gdlint-exclude-folders branch from 023173a to e0047f4 Compare March 8, 2024 02:36
@chrisl8
Copy link
Contributor Author

chrisl8 commented Mar 8, 2024

Updated code is ready.

Copy link
Owner

@Scony Scony left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

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

Successfully merging this pull request may close these issues.

2 participants