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

Add Resistor Color Trio Exercise #193

Merged
merged 7 commits into from
Dec 13, 2019
Merged

Add Resistor Color Trio Exercise #193

merged 7 commits into from
Dec 13, 2019

Conversation

soumitradev
Copy link
Contributor

Added previously unimplemented Resistor Color Trio Exercise

Fixed minor issue where example.jl printed the result instead of returning it
@codecov
Copy link

codecov bot commented Dec 9, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@f3b9406). Click here to learn what that means.
The diff coverage is 98.57%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #193   +/-   ##
=========================================
  Coverage          ?   96.41%           
=========================================
  Files             ?      120           
  Lines             ?     1199           
  Branches          ?        0           
=========================================
  Hits              ?     1156           
  Misses            ?       43           
  Partials          ?        0
Impacted Files Coverage Δ
...ercises/resistor-color-trio/resistor-color-trio.jl 0% <0%> (ø)
exercises/resistor-color-trio/example.jl 100% <100%> (ø)
exercises/minesweeper/runtests.jl 100% <100%> (ø)
exercises/minesweeper/example.jl 100% <100%> (ø)
exercises/resistor-color-trio/runtests.jl 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3b9406...b45ad53. Read the comment docs.

@SaschaMann
Copy link
Contributor

SaschaMann commented Dec 10, 2019

Thanks for adding the exercise!

I like that you commented the example solution :)

I think this exercise is a really good candidate to go beyond the canonical data and use Unitful.jl. Returning units as strings seems rather unjulian and it would be a pretty nice non-core exercise for people to play around with.

Do you want to you convert it to use Unitful and provide a Project.toml & Manifest.toml file for a reproducible environment for the student? I'll leave the decision up to you, I think the exercise is interesting as it is already, too.

The repo's runtests.jl file can't really handle dependencies yet, but I can fix that afterwards as long as the tests of this PR pass locally.

--

Two things about the coding style:

  • In Julia it's preferred that variables have snake_case names rather than camelCase. It only affects internal variables here, so it's not a big deal, but it would be best if you can adjust them
  • Please add a trailing newline to all files. That makes it easier to read diffs/changes later on. Most editors have a setting to do this automatically.

config.json Outdated Show resolved Hide resolved
soumitradev and others added 3 commits December 12, 2019 00:04
Co-Authored-By: Sascha Mann <git@mail.saschamann.eu>
Forgot to run generate_notebooks.jl causing 1/21 tests to fail. Oops!
@soumitradev
Copy link
Contributor Author

I think this exercise is a really good candidate to go beyond the canonical data and use Unitful.jl. Returning units as strings seems rather unjulian and it would be a pretty nice non-core exercise for people to play around with.

Do you want to you convert it to use Unitful and provide a Project.toml & Manifest.toml file for a reproducible environment for the student? I'll leave the decision up to you, I think the exercise is interesting as it is already, too.

Sure! I'd love to do that in the future and work on making the exercise more interesting and clear, but for now, I think I'll leave that for later when I have time.

Two things about the coding style:

  • In Julia it's preferred that variables have snake_case names rather than camelCase. It only affects internal variables here, so it's not a big deal, but it would be best if you can adjust them

Don't the Code Formatting Guidelines on the README suggest camelCase for types? Which one should I use?

  • Please add a trailing newline to all files. That makes it easier to read diffs/changes later on. Most editors have a setting to do this automatically.

Sure, I'll just do this in the next commit along with the case change I mentioned.

@SaschaMann
Copy link
Contributor

Sure! I'd love to do that in the future and work on making the exercise more interesting and clear, but for now, I think I'll leave that for later when I have time.

Okay, then I'll merge this once the formatting/style changes have been done and you can pick it up later whenever you want.

Don't the Code Formatting Guidelines on the README suggest camelCase for types? Which one should I use?

Type names, e.g. Int, String, struct MyOwnType; end should be upper camel case. Variable and function/method names should be lower case with additional underscores if needed.

So for example in your code:

function label(colors::AbstractArray)

    # Set the color-number converter list
    colorKey = ["black", "brown", "red", "orange", "yellow", "green", "blue", "violet", "grey", "white"]

AbstractArray is a type name -> it's upper camel case
label is a method/function name -> it's lower case
colorKey is a variable name -> it should be lowercase colorkey or color_key

Changed camelCase to snake_case for variables, and added trailing newline at the end of every file.
@SaschaMann
Copy link
Contributor

SaschaMann commented Dec 13, 2019

Thanks!

@soumitradev Please leave a short comment whenever you add a new commit that addresses changes to a PR. Otherwise GitHub won't send a notification and it might go unnoticed.

@SaschaMann SaschaMann merged commit 54ef80e into exercism:master Dec 13, 2019
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.

None yet

2 participants