-
-
Notifications
You must be signed in to change notification settings - Fork 110
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
Add practice exercise pop-count
#617
Conversation
Oops, I had elm-review suggest pruning unneeded dependencies from elm.json and didn't think twice about it. |
Yeah, this is a bi of an annoyance. All the elm.json files must be the same, its a requirement of the test runner, which has to makle sure that it has all the dependencies in the elm.json files. |
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.
This all looks good to me. I'll let Jie / Matthieu take a look as well, and you'll need to fix up the elm.json in order for the build to pass. I'll approve when it does. The tests can be flaky, so if that happens let me know and I'll re run them.
Thanks for the contribution!
Cheers, Cedd
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.
I'm good with this as well (once the things already discussed are settled), thank you for porting this.
I was wondering if we needed an analyzer comment to prevent people from using a built-in popCount
, but there is no such function in Bitwise
and I think elm/bytes
is also pretty unrelated, so we should be good.
"test_runner": true, | ||
"representer": true, | ||
"analyzer": true | ||
}, |
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.
That's too many changes on the config, I can't be sure of what the actual change is. I'm assuming it got sorted somehow, but could you revert that?
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.
I had to run configlet fmt to make the CI pass. There was a recent release for configlet so I think that’s the issue. I can open a separate PR for the config formatting. That’ll make it easier to see the actual changes.
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.
Oh I see, thank you for pointing it out. Please leave it like that, I checked that it's only adding what it's supposed to.
But that's strange, when I run bin/configlet fmt
is says everything is clear, but running bin/configlet fmt --update
on main it changes this file. Is it the same for you? I should flag that.
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.
Thank you for adding this one :)
Will close #615