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

Fixes #3

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

Fixes #3

wants to merge 8 commits into from

Conversation

feld
Copy link

@feld feld commented Mar 13, 2024

various fixes as I go along testing this library for a project

Copy link
Member

@half-shell half-shell left a comment

Choose a reason for hiding this comment

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

Hi @feld 👋
Thank you for your contribution!

I submitted a couple of comments related to the majority of the lines you changed. Ideally, dropping e734ef1 and 50d697a would be the easiest way of dealing with them 🙂

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).

Comment on lines +1 to +3
[
inputs: ["mix.exs", "{config,lib,test}/**/*.{ex,exs}", "priv/repo/migrations/*.exs", "priv/repo/optional_migrations/**/*.exs"]
]
Copy link
Member

Choose a reason for hiding this comment

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

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.

Comment on lines +25 to +26
{:credo, "~> 1.7", only: :dev},
{:dialyxir, "~> 1.4", only: :dev},
Copy link
Member

Choose a reason for hiding this comment

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

We are not planning on using credo nor dialyxir, even just as dev dependencies. We might in the future, it is just not something that we want to do in isolation, in this repo only.

We currently are in the process of cleaning up our repos, and decided that this kind of change would be discussed down the line.

We'll gladly add those later on if it is something we decide to do as an organization 🙂

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

Successfully merging this pull request may close these issues.

2 participants