Skip to content

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

Merged
merged 5 commits into from
May 4, 2025

Conversation

pozdniakovda
Copy link
Contributor

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.

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.
Copy link
Contributor

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.

[no important files changed]

For more information, refer to the documentation. If you are unsure whether to add the message or not, please ping @exercism/maintainers-admin in a comment. Thank you!

Copy link
Member

@ryanplusplus ryanplusplus left a 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];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
aminoacid_t protein[MAX_AMINOACIDS];
amino_acid_t amino_acids[MAX_AMINOACIDS];

Copy link
Member

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

Copy link
Contributor Author

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 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
aminoacid_t protein[MAX_AMINOACIDS];
aminoacid_t protein[MAX_AMINO_ACIDS];

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
aminoacid_t protein[MAX_AMINOACIDS];
amino_acid_t protein[MAX_AMINO_ACIDS];

Copy link
Contributor Author

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)
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

@pozdniakovda pozdniakovda Apr 18, 2025

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?

Copy link
Member

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.

Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

Choose a reason for hiding this comment

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

No worries!

@ryanplusplus ryanplusplus merged commit 61e26b9 into exercism:main May 4, 2025
3 of 4 checks passed
@pozdniakovda pozdniakovda deleted the patch-1 branch May 7, 2025 00:47
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.

2 participants