-
Notifications
You must be signed in to change notification settings - Fork 2
[LTB-65] Add Buildable for ConfigRec #56
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of small suggestions, but LGTM.
Ah one more thing, I think that the commit message for b4e16fe has a typo: Also, there's no written rule AFAIK, but in several projects for these commits that are not directly related to an issue we use the "tag" |
https://www.merriam-webster.com/dictionary/indention
|
Problem: 1. Our style guide requires two spaces indentation, but in some places we have four or even more spaces. 2. Someone may have different formatter settings but we do not want someone reformat our code just saving file. Solution: 1. Delete spaces where it contradicts our style guide. 2. Add stylish-haskell config.
e60e28b
to
b8b0064
Compare
@pasqu4le if you want, you can take a look at requested changes |
oh I didn't know that, I assumed it was a typo. No problem then 👌 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now 👍
Problem: This was made here #53. But this PR became still. It'll be good to proceed this PR and add Buildable instances, cause it's quite useful. We not going to merge #53, because Json instances was added recently independent from this PR (we didn't know about it). Solution: Move Buildable instances (along with tests) form #53.
b8b0064
to
b516300
Compare
Problem:
This was made here #53. But this PR became still.
It'll be good to proceed this PR and add Buildable instances, cause it's quite useful.
We're not going to merge #53, because Json instances was added recently independent from this PR (we didn't know about it).
Solution:
Move Buildable instances (along with tests) form #53.
Also in thins PR presented one stylistic commit with formatter config.