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

Improve maintainability #70

Merged
merged 4 commits into from
Jan 14, 2025

Conversation

edouardmenayde
Copy link
Contributor

@edouardmenayde edouardmenayde commented Jan 8, 2025

Fixes some problems I encountered while contributing on my last PR :

  • mix format was not up-to-date, I executed it and added a check in the CI
  • Fix two warnings (most of the rest is double @doc and depreciations)
  • CI would not run on pull request

To be reviewed commit by commit for easier reading

@Ninigi Ninigi self-requested a review January 9, 2025 07:20
Comment on lines -15 to -18
iex> url = "https://api.ibanity.com/customer/synchronizations"
...> Ibanity.IdReplacer.replace_all(url, &String.replace("*", String.length(&1)))
"https://api.ibanity.com/customer/synchronizations"

Copy link

Choose a reason for hiding this comment

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

I'm not sure what this was supposed to test, but I can see that it wouldn't work because String.replace/2 isn't a function...

Since this is in internal module, and the only way it is used is IdReplacer.replace_all(&Recase.to_snake/1) , I agree with removing the faulty test case. 👍

@Ninigi
Copy link

Ninigi commented Jan 9, 2025

@edouardmenayde thanks for the PR, all the warnings and not canonically formatted code has been bugging me too 👍

Question before merging: are you planning to get rid of the duplicate @doc warnings in this PR as well, or is that out of scope for you? I'm just asking so I know if I should wait before merging.

@edouardmenayde
Copy link
Contributor Author

@edouardmenayde thanks for the PR, all the warnings and not canonically formatted code has been bugging me too 👍

Question before merging: are you planning to get rid of the duplicate @doc warnings in this PR as well, or is that out of scope for you? I'm just asking so I know if I should wait before merging.

I am unsure on why it was written this way, so I didn't want to touch it.
I guessed you might be using it to generate doc outside hex doc, and it might break it.

@edouardmenayde
Copy link
Contributor Author

Could this be merged and a new release be published ?

@Ninigi
Copy link

Ninigi commented Jan 14, 2025

I'll look into a new release, but not sure it will be up today.

@Ninigi Ninigi merged commit f2a73b3 into ibanity:master Jan 14, 2025
1 check passed
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