Skip to content
This repository has been archived by the owner on Jun 30, 2021. It is now read-only.

Upgrade credo to 0.10.0 #433

Merged
merged 10 commits into from
Sep 3, 2018
Merged

Upgrade credo to 0.10.0 #433

merged 10 commits into from
Sep 3, 2018

Conversation

dsdshcym
Copy link
Contributor

Issue/Task Number: fixes #362

Overview

This PR upgrades credo to 0.10.0

Changes

See commits list

Implementation Details

This PR deactivates the newly introduced checker Credo.Check.Readability.AliasOrder, which I think it's not that useful.

And one of aeternity projects also deactivates it as well: https://github.com/aeternity/elixir-node/blob/5c47e8b/.credo.exs#L77

@dsdshcym
Upgrade credo to 0.9.3 (omgnetwork#362)
4b679b4
@dsdshcym
Deactivate Credo.Check.Readability.AliasOrder checker (omgnetwork#362)
e5270d4 
- AliasOrder may become a burden when we want to order aliases in a
  specific way, IMO
- And we've already violated it in over 200 modules
@dsdshcym
Upgrade credo to 0.10.0 (omgnetwork#362)
c6b74cb
@dsdshcym
Fix credo warning about preferring Enum.into/3 over into/2 (omgnetwor…
76a3dad 
…k#362)

```
  Refactoring opportunities
┃
┃ [F] ↗ `Enum.into/3` is more efficient than `Enum.map/2 |> Enum.into/2`
┃       apps/local_ledger/lib/local_ledger/cached_balance.ex:59 #(LocalLedger.CachedBalance.add_amounts)
┃ [F] ↗ `Enum.into/3` is more efficient than `Enum.map/2 |> Enum.into/2`
┃       apps/ewallet/lib/ewallet/web/websocket.ex:107 #(EWallet.Web.WebSocket.normalize_headers)
┃ [F] ↗ `Enum.into/3` is more efficient than `Enum.map/2 |> Enum.into/2`
┃       apps/ewallet/lib/ewallet/web/v1/error_handler.ex:427 #(EWallet.Web.V1.ErrorHandler.error_fields)
```
@T-Dnzt
Copy link

T-Dnzt commented Aug 24, 2018

@dsdshcym Thanks for submitting this PR! However, we do like the AliasOrder idea. Could you leave it enabled?

@unnawut unnawut added this to the v1.1 milestone Aug 29, 2018
@unnawut unnawut added the kind/enhancement 🚀 New feature or request label Aug 29, 2018
@ghost ghost assigned unnawut Aug 29, 2018
@ghost ghost added the s2/wip 🚧 label Aug 29, 2018
@unnawut
Copy link
Contributor

unnawut commented Aug 29, 2018

Since it's a small change request I took the courtesy to update it just now so it can proceed 🍾

@unnawut unnawut requested a review from T-Dnzt August 29, 2018 06:37
@unnawut
Copy link
Contributor

unnawut commented Aug 29, 2018 •
edited

I see what happened. So removing the Credo.Check.Readability.AliasOrder produces a large bunch of errors. Those seem straightforward to fix though, and it's a lot easier to eye-scan through the list of aliases when they're ordered so I'd like to keep the ordering too.

@dsdshcym if you'd like to tackle those please let us know in a couple of days.

@dsdshcym
Copy link
Contributor Author

@unnawut There are still about 110 warnings after e384514. I'll try to fix them this weekend.

@dsdshcym
Copy link
Contributor Author

@unnawut @T-Dnzt I've fixed all the remaining warnings in 0bbce54. Please check :)

unnawut
unnawut suggested changes Aug 31, 2018
View reviewed changes
Copy link
Contributor

@unnawut unnawut left a comment

Choose a reason for hiding this comment

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

Great work!

I noticed that some aliases were grouped and some were left ungrouped. Could you merge these ones I commented?

Apart from that, looks good to merge!

apps/admin_api/lib/admin_api/v1/controllers/transaction_consumption_controller.ex Outdated Show resolved Hide resolved
apps/local_ledger_db/test/local_ledger_db/entry_test.exs Outdated Show resolved Hide resolved
apps/local_ledger_db/test/local_ledger_db/token_test.exs Outdated Show resolved Hide resolved
apps/local_ledger_db/test/local_ledger_db/transaction_test.exs Outdated Show resolved Hide resolved
apps/local_ledger_db/test/local_ledger_db/wallet_test.exs Outdated Show resolved Hide resolved
@unnawut
Copy link
Contributor

unnawut commented Sep 1, 2018

Thank you! 🎖🥇🍾 🌟 ❤️

@unnawut unnawut merged commit fe6d1dc into omgnetwork:master Sep 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@unnawut unnawut unnawut approved these changes

@T-Dnzt T-Dnzt Awaiting requested review from T-Dnzt

Assignees

@unnawut unnawut

@T-Dnzt T-Dnzt

Labels
kind/enhancement 🚀 New feature or request
Projects
None yet
Milestone
v1.1
Development

Successfully merging this pull request may close these issues.

Upgrade Credo to 0.9.3
3 participants
@dsdshcym @T-Dnzt @unnawut