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 #53

Merged
merged 4 commits into from
Feb 28, 2024

Conversation

glaxxie
Copy link
Contributor

@glaxxie glaxxie commented Feb 19, 2024

First timer 🙏

Fix spacing so methods are 2 empty lines apart.
Copy link
Member

@BNAndras BNAndras left a comment

Choose a reason for hiding this comment

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

Looks good to me. @pfertyk, at some point we should sort the exercises by difficulty group as shown on the website (I believe it's 1-3 for easy, 4-6 for medium, 7-10 for hard) and then by exercise name within each group. The reason I mention this is that the resistor-color series is a notable exception where it'd be better to sort them sequentially. That lets the student see how each step leads into the next one from resistor-color to resistor-color-trio

@BNAndras
Copy link
Member

Another thing to float. Do we want the inputs to be arrays of colors or do we want to require three arguments, one for each color. The latter would make the last test about ignoring additional colors unnecessary. All the other tests use three colors.

At the moment, the ported resistor-color-duo uses an array of colors so we should be consistent, but that doesn't preclude us from changing resistor-color-duo to use two arguments, one for each color.

@pfertyk
Copy link
Contributor

pfertyk commented Feb 26, 2024

@BNAndras noted, I already try to sort the exercises by difficulty, but we can update the whole list later 👍

As to arrays vs separate arguments, I am OK with both solutions. I would, however, lean a bit toward an array, as it will be consistent with the Python track. GDScript is very similar to Python, and I like to keep the solutions similar to make things easier for people who already completed the exercises in Python.

On the other hand, separate arguments will be "consistent" with the first exercise of this series (which accepts a single argument, not a single element array) :P What are your thoughts @glaxxie ?

@BNAndras
Copy link
Member

Yeah, I wouldn’t have thought of it but the Julia maintainers mentioned it when I ported resistor-color-duo. They still ended up using an array because resistor-color-trio was already ported that way. I think the array approach is pretty typical from what I’ve seen in other tracks.

@glaxxie
Copy link
Contributor Author

glaxxie commented Feb 28, 2024

Fixed the spacing issue. I didn't use the godot editor at first since I was fiddling about for far too long, so I used vscode for the first PR.
The tab there is configured to be 4 spaces, was a bit surprised to see actual tab is 8 spaces on github.

Regarding the point about using array vs args: I would favor array as well for the subsequent resistor exercises.

I actually did have a conversation related this when code review with a learner. They asked why in the resistor expert version they should return '0 ohms' early if there is only one color in the array, and what if it was green. I dig around and the all the results that show resistors having one single band that I can found is the black one and nothing else.

From that, we can rationalize that the very first exercise is more about introducing the idea of assigning a color to retrieve a certain value so using one arg (string) here is ok, and not really an accurate portrait of how resistor should be like later ones (array of colors), just like there isn't one with a leading black color (0) irl so there is no need to test for them either.

Edit: I also have the resistor expert exercise practically done, just wait for this to clear first.

@BNAndras
Copy link
Member

Yeah, that GitHub thing led to a few issues for me on the first few PRs until I got the hang of it. :)

Fix another spacing issue
@pfertyk pfertyk merged commit f2687a1 into exercism:main Feb 28, 2024
2 checks passed
@pfertyk
Copy link
Contributor

pfertyk commented Feb 28, 2024

Thank you for contributing @glaxxie ! :)

@glaxxie glaxxie deleted the gdscript_resistor_color_trio branch February 28, 2024 20:25
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.

3 participants