Skip to content

Implement exercise protein-translation #585

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

Closed
wants to merge 2 commits into from
Closed

Implement exercise protein-translation #585

wants to merge 2 commits into from

Conversation

clementi
Copy link

@clementi clementi commented Aug 2, 2017

No description provided.

@levex
Copy link

levex commented Aug 2, 2017

Why is this commit empty?

@clementi
Copy link
Author

clementi commented Aug 2, 2017

The section "Avoiding Duplicate Work" in the document "Porting an Exercise to Another Language Track" indicates that one should create an empty WIP PR to indicate that you're working on the particular exercise. Maybe I've done this incorrectly?

@levex
Copy link

levex commented Aug 2, 2017

Apologies, I wasn't aware of that!

@clementi clementi changed the title [WIP] Implement exercise protein-translation Implement exercise protein-translation Aug 16, 2017
Fix typo

Fix typo

Implement exercise

Genericize package.yaml

Add protein-translation to config.json

Simplify expression

Provide for invalid codon
@clementi
Copy link
Author

This exercise has now been implemented and the PR is ready for review.

@clementi
Copy link
Author

clementi commented Aug 26, 2017

@kytrinyx or @levex or whoever, this is ready for review.

Sent from my Motorola XT1575 using FastHub

@kytrinyx kytrinyx requested a review from a team September 1, 2017 14:08
@kytrinyx
Copy link
Member

kytrinyx commented Sep 1, 2017

Thank you @clementi! I've pinged the Haskell track maintainers so that they can take a look at it.

@levex If you have a few moments to spare to provide your input that would be valuable as well. ✨

@@ -780,7 +789,7 @@
"uuid": "d5997b60-e54c-4caa-beae-8614f0da3fb3",
"slug": "trinary",
"deprecated": true
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Would you run configlet fmt . using the latest release of our Configlet tool? It helps us ensure that tracks are configured properly and normalized.

"core": false,
"unlocked_by": null,
"difficulty": 1,
"topics": [
Copy link
Member

Choose a reason for hiding this comment

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

Do you have any suggestions about what topics this exercise might touch upon?

We have a list of some common topics in https://github.com/exercism/problem-specifications/blob/master/TOPICS.txt - but you are not restricted to the topics in that list. If there are any new Haskell-specific topics or language-features that this exercises uses, those would be a good choice.

toProtein :: String -> [String]
toProtein strand = takeWhile ("STOP" /=) $ map codonToProtein $ chunksOf 3 strand

codonToProtein :: String -> String
Copy link

Choose a reason for hiding this comment

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

Just a quick question, maybe I'm missing something, but to me the description is a little misleading. It seems that the correct solution would be just to do the map operation written in the description, but judging by this and the test cases it seems that it actually expects the protein names instead of the codons?

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Member

@petertseng petertseng left a comment

Choose a reason for hiding this comment

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

Thanks.

@@ -782,7 +791,7 @@
"uuid": "d5997b60-e54c-4caa-beae-8614f0da3fb3",
"slug": "trinary",
"deprecated": true
}
}
Copy link
Member

Choose a reason for hiding this comment

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

please remove these spaces, as they have nothing to do with adding the exercise

"slug": "protein-translation",
"core": false,
"unlocked_by": null,
"difficulty": 1,
Copy link
Member

Choose a reason for hiding this comment

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

I was going to ask about difficulty 1 because I didn't think this exercise is difficulty 1, but given that run-length-encoding is also difficulty 1, I guess I can't complain.

@@ -0,0 +1,79 @@
# Rna Transcription
Copy link
Member

Choose a reason for hiding this comment

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

this is the description for the RNA transcription exercise (https://github.com/exercism/problem-specifications/blob/master/exercises/rna-transcription/description.md), whereas given that this is in the protein-translation directory it needs to be the description of the protein translation exercise: https://github.com/exercism/problem-specifications/blob/master/exercises/protein-translation/description.md

@@ -0,0 +1,20 @@
name: protein-translation
version: 1.0.0.0
Copy link
Member

Choose a reason for hiding this comment

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

given that https://github.com/exercism/problem-specifications/tree/master/exercises/protein-translation/canonical-data.json does not exist (that link will lead to a 404), the version number that is in accordance with the versioning policy (#522) will be 0.1.0.1 rather than 1.0.0.0

, strand = "UUG"
, expected = ["Leucine"]
}
, Case { description = "identifies leucine codon (UUG)"
Copy link
Member

Choose a reason for hiding this comment

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

is this case any different from the one immediately above? I see two "identifies leucine codon (UUG)"

, strand = "AUGUUUUAA"
, expected = ["Methionine", "Phenylalanine"]
}
, Case { description = "stops translation of longest strand"
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand what "longest" means in this context. Can you clarify? I know that it is the longest strand used in the test cases so far, but I don't understand why that must be specified in the description. To me the important part of this test case is that even if there are codons after STOP, they are not included in the output. Shouldn't that be the description?

@petertseng
Copy link
Member

I regret that between the dates of August 26 and September 15 (a period of 20 days), it was ready for review, but no maintainer of this track reviewed it.

An >= equivalent period of time (24 days) has passed since my review, and we had unanswered questions about the duplicate description "identifies leucine codon (UUG)" and the meaning of "stops translation of longest strand", which we need answered before being able to merge it. We await your answer, but to get the PR out of the queue for now, we are closing it.

Despite it being closed, we are still open to receiving the answers, and would gladly reopen it upon receiving them. Feel free to reopen it by yourself if the permissions allow that (not familiar enough to know whether it is) or ask us to and we will.

@Average-user
Copy link
Member

Can I implement this?

@petertseng
Copy link
Member

@Average-user
Copy link
Member

ok, But I'll make a new PR.

@petertseng
Copy link
Member

Indeed, unfortunately it is not possible to transfer ownership of a PR to someone else, so you will indeed have to make a new one.

@Average-user
Copy link
Member

Oh, didn't now that

@Average-user
Copy link
Member

@petertseng Should this be String -> [String] or String -> Maybe [String]?

@petertseng
Copy link
Member

Ah, because there are strings that could not translate to any sequence of proteins? Okay, I suggest String -> Maybe [String] then. I note the canonical data doesn't have any case that would result in None, so you are not obligated to add any, but would still use Maybe nevertheless.

Thinking about String -> Maybe [Protein] though? Note sure whether I feel like making an ADT for these proteins. Try it out and see what the two options look like, then say why you decided on your choice.

@Average-user
Copy link
Member

Mmm ADT are fun, I'll see

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.

5 participants