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 protein translation #659

Merged
merged 5 commits into from
Feb 24, 2024
Merged

Conversation

BNAndras
Copy link
Sponsor Member

Related to #655

Copy link
Contributor

@jiegillet jiegillet left a comment

Choose a reason for hiding this comment

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

Very cool, thank you @BNAndras!

There are some finishing touches needed, but I can't see any actual problem :)

config.json Outdated Show resolved Hide resolved
exercises/practice/protein-translation/tests/Tests.elm Outdated Show resolved Hide resolved
test "Non-existing codon can't translate" <|
\() ->
ProteinTranslation.proteins "AAA"
|> Expect.equal (Err "Invalid codon")
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use types all the way, I think it's the most idiomatic way.

Suggested change
|> Expect.equal (Err "Invalid codon")
|> Expect.equal (Err InvalidCodon)

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

I've made the other edits, but I'll need to work my way through adding the types and chasing out the ramifications. I haven't touched Elm in a while so I'm a little rusty. I can poke at it more this weekend.

module ProteinTranslation exposing (proteins)


proteins : String -> Result String List todo
Copy link
Contributor

Choose a reason for hiding this comment

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

The types need to be adjusted

Ok []

codons ->
case processCodons [] codons of
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if it's better, but I think this whole block could be

codons
 |> processCodons []
 |> Result.map List.reverse

and if you could somehow treat the empty case in processCodons the whole function could be

strand
 |> splitIntoCodons
 |> processCodons []
 |> Result.map List.reverse

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

Thanks for the review, I'll make the edits tomorrow.

@jiegillet
Copy link
Contributor

@BNAndras do you still plan on finishing this? I can take over if you like.

@BNAndras
Copy link
Sponsor Member Author

Sorry, I was working on the LFE track this weekend. I'll take a look tonight.

@BNAndras
Copy link
Sponsor Member Author

I’m under the weather so if you wrap this up, I’ll owe you one. :)

@jiegillet jiegillet merged commit 32d3603 into exercism:main Feb 24, 2024
6 checks passed
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