-
-
Notifications
You must be signed in to change notification settings - Fork 193
Exercises - Protein Translation - Patch 1 (to clarify biological terminology) #1048
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
Conversation
Renamed typedef statements to better reflect biological terminology: Methionine, Serine, etc. are examples of amino acids, and a protein is a sequence of several amino acids. A single RNA strand corresponds to one protein, not several.
proteins_t => protein_t A single RNA strand corresponds to a single protein.
Renamed typedef statements to better reflect biological terminology: Methionine, Serine, etc. are examples of amino acids, and a protein is a sequence of several amino acids. A single RNA strand corresponds to one protein, not several.
Renamed some variables and type definitions to match biological terminology: one codon corresponds to one amino acid, and one RNA sequence of several amino acids corresponds to one protein.
This PR touches files which potentially affect the outcome of the tests of an exercise. This will cause all students' solutions to affected exercises to be re-tested. If this PR does not affect the result of the test (or, for example, adds an edge case that is not worth rerunning all tests for), please add the following to the merge-commit message which will stops student's tests from re-running. Please copy-paste to avoid typos.
For more information, refer to the documentation. If you are unsure whether to add the message or not, please ping |
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.
Thanks this looks good overall. I just have a few suggested tweaks to improve naming consistency (aminoacid
-> amino_acid
) and to further clarify the structure of a protein.
|
||
typedef struct { | ||
bool valid; | ||
size_t count; | ||
protein_t proteins[MAX_PROTEINS]; | ||
} proteins_t; | ||
aminoacid_t protein[MAX_AMINOACIDS]; |
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.
aminoacid_t protein[MAX_AMINOACIDS]; | |
amino_acid_t amino_acids[MAX_AMINOACIDS]; |
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 think this makes it clearer that a protein consists of amino acids
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.
A protein can be viewed as an array of amino acids, so calling for array named protein[MAX_AMINO_ACIDS] (including earlier fix in const name) made up of variables of type amino_acid_t makes much more sense to me :)
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.
aminoacid_t protein[MAX_AMINOACIDS]; | |
aminoacid_t protein[MAX_AMINO_ACIDS]; |
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.
aminoacid_t protein[MAX_AMINOACIDS]; | |
amino_acid_t protein[MAX_AMINO_ACIDS]; |
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.
The latter one is the correct one.
static void test_sequence_of_two_protein_codons_translates_into_proteins(void) | ||
static void test_sequence_of_two_protein_codons_translates_into_protein(void) |
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 test name comes from https://github.com/exercism/problem-specifications/blob/75e16b1e51d126443dfd85f94e5d7cf4b5dbfd9d/exercises/protein-translation/canonical-data.json#L172. Can you make a pull request in https://github.com/exercism/problem-specifications to fix the test names upstream?
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.
Technically this is my first ever PR, could you clarify if it's possible to add more commits to the ongoing PR? I could add a commit regarding that file, ..../canonical-data.json to this PR if it's possible.
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.
Technically this is my first ever PR, could you clarify if it's possible to add more commits to the ongoing PR? I could add a commit regarding that file, ..../canonical-data.json to this PR if it's possible.
Yep, if you make more commits the pull request will auto-update.
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 a look at the repo called problem-specifications. It looks like these problems are shared across all language tracks. Does it mean changing the file canonical-data.json will affect this exercise across all languages?
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.
Yes. Technically it's not required that the other tracks accept the name change, but given that the name is incorrect (or at least misleading) I think it's worth proposing the change.
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.
@pozdniakovda do you need help updating this PR with your 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.
Apologies for a late reply. I'd appreciate a tip, I'm unsure how exactly to commit these changes as discussed using GitHub web interface.
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.
You can use the "Commit suggestion" button to apply it. If you're doing this from the "Files changed" tab, you can add these to a batch and apply them all at once.
Alternately, you can check out your branch on your computer and manually apply the changes, commit, and push the commit.
If you're struggling with these options, I can also make the changes to your PR myself.
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.
Thanks for finalizing these changes. Work got my attention, and I forgot to respond in time.
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.
No worries!
According to how the exercise's stub files and test files are currently written, one could assume that from a single RNA strand multiple proteins are decoded. In the example files, stub files and test file I made a few minor changes to the names of types, variables, etc. to more accurately reflect biological terminology, e.g., one 3-letter codon corresponds to one amino acid (Methionine, Phenylalanine, etc.), and one RNA sequence corresponds to one protein, which is made out of several amino acids. Feedback on this PR will be greatly appreciated.