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

Fix warnings for Elixir 1.17 by formatting the project using mix format #101

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

wkirschbaum
Copy link

@wkirschbaum wkirschbaum commented Jun 4, 2024

This PR adds a .formatter.exs file and applies changes when running mix format. Additionally mix deps.update --all has been run to ensure we don't receive warnings for older versions of dependencies.

@half-shell
Copy link
Member

Hi @wkirschbaum 👋

Thank you for you contribution, it is very much appreciated !

However, we avoid using mix's formatter for repos that don't use it from the beginning, reason being that the changes they apply don't have any functionnal meaning, and we try to keep an as obvious and simple as possible in the git history.

I know it is not obvious on the repository scope. We currently are in the process of smoothing out all of our public repos, and as such they don't all have a "Contributing" section in their README.md yet, or a link the our organization's one, where we have a section related to how we handle changes related to formatting commits (relevant section here).

I'll be more than happy to merge your PR with the two formatting-related commits dropped.

Thanks again !

@wkirschbaum
Copy link
Author

@half-shell thanks for the maintenance of this library.

The formatting is to get rid of the warnings. Either we format it manually, or we use the tool for less risk of messing something up. if you want to manually only change the '' to ~c"", but unfortunately I don't feel it is time well spent.

The two formatting commits is the purpose of this PR, so omitting it defeats the point 😆.

I won't be offended if you reject this PR :), then perhaps this PR can be viewed as an issue instead to solve warnings for 1.17.

@half-shell
Copy link
Member

You're right, and I should have been clearer in my answer.

What I meant was to avoid mass formatting over a whole repo. The warnings you mention only relate to 5 lines, so what I'm arguing for is rather to have a commit "remove several warnings regarding the use of charlists instead of strings" with only those lines changed 🙂

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