palindrome-products: require factors in solution #2006

ellnix · 2025-01-30T10:06:06Z

Apologies if I did something wrong, struggled a little bit with the rust-tooling.

Please let me know of any mistake, miss, or decisions you disagree with, no matter how small. No hard feelings.

The HashSet

Used a HashSet<(u64, u64)> for factors to avoid dealing with ordering of the factors and simultaneously communicate that the order doesn't matter. I thought leaving the inner tuple sorted made sense since that's how it appears in the instructions:

[...] The largest palindrome product is 9. Its factors are (1, 9) and (3, 3).

A sorted Vec would probably be fine, and the change can be made easily enough if you think it would be more appropriate.

The extra tests

There were two extra tests on the now-defunct Palindrome::new function. I felt those only made sense if this exercise still contained the newtype pattern. I can add them again if appropriate.

Corresponding forum post: https://forum.exercism.org/t/palindrome-products-description-mentions-factors/

github-actions · 2025-01-30T10:06:19Z

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!

github-actions · 2025-01-30T10:06:20Z

Hello. Thanks for opening a PR on Exercism 🙂

We ask that all changes to Exercism are discussed on our Community Forum before being opened on GitHub. To enforce this, we automatically close all PRs that are submitted. That doesn't mean your PR is rejected but that we want the initial discussion about it to happen on our forum where a wide range of key contributors across the Exercism ecosystem can weigh in.

You can use this link to copy this into a new topic on the forum. If we decide the PR is appropriate, we'll reopen it and continue with it, so please don't delete your local branch.

If you're interested in learning more about this auto-responder, please read this blog post.

Note: If this PR has been pre-approved, please link back to this PR on the forum thread and a maintainer or staff member will reopen it.

github-actions · 2025-01-30T10:06:20Z

Hello 👋 Thanks for your PR.

This repo does not currently have dedicated maintainers. Our cross-track maintainers team will attempt to review and merge your PR, but it will likely take longer for your PR to be reviewed.

If you enjoy contributing to Exercism and have a track-record of doing so successfully, you might like to become an Exercism maintainer for this track.

Please feel free to ask any questions, or chat to us about anything to do with this PR or the reviewing process on the Exercism forum.

(cc @exercism/cross-track-maintainers)

ellnix · 2025-01-30T10:08:12Z

exercises/practice/palindrome-products/.meta/test_template.tera

+      {%- endif%}
+      assert_eq!(pal.value(), {{ test.expected.value }});
+      assert_eq!(pal.factors(), &HashSet::from([
+        {{- test.expected.factors | join(sep=", ") | replace(from="[", to="(") | replace(from="]", to=")") -}}


From the last PR:

I also had this instead of the one-liner:

Suggested change

{{- test.expected.factors | join(sep=", ") | replace(from="[", to="(") | replace(from="]", to=")") -}}

{%- for factor in test.expected.factors -%}

({{ factor | join(sep=", ") }}){% if not loop.last %}, {% endif %}

{%- endfor -%}

I didn't really like either of them, but thought I'd drop this here just in case. There's probably some jinja-fu that can be done instead.

I'm not too worried about the beauty of these templates. It seems like a write-only thing to me. If the exercise is changed substantially, there's a good chance the test template will have to be done basically from scratch.

Fair enough, as someone who has tried to make tera templates pretty in the past, I can say you have the right attitude.

senekor · 2025-01-30T10:11:26Z

struggled a little bit with the rust-tooling

Yeah that's something I cooked up on my own. What were you struggling with? It could point us to potential improvements or at least missing documentation. Did you stumble on docs/CONTRIBUTING.md#tooling?

ellnix · 2025-01-30T10:18:55Z

struggled a little bit with the rust-tooling

Yeah that's something I cooked up on my own. What were you struggling with? It could point us to potential improvements or at least missing documentation. Did you stumble on docs/CONTRIBUTING.md#tooling?

Not at all, honestly there's a ton of documentation on exercism in general and the tracks specifically that getting started on doing something is a bit of a challenge. But, it looks like I did exactly what I was supposed to, read a bunch of source code, created the problem-specifications symlink myself after investigating why things weren't working.

I'll make sure to read everything properly in the future.

senekor · 2025-01-30T10:25:19Z

exercises/practice/palindrome-products/.meta/test_template.tera

+      {%- endif%}
+      assert_eq!(pal.value(), {{ test.expected.value }});
+      assert_eq!(pal.factors(), &HashSet::from([
+        {{- test.expected.factors | join(sep=", ") | replace(from="[", to="(") | replace(from="]", to=")") -}}


I'm not too worried about the beauty of these templates. It seems like a write-only thing to me. If the exercise is changed substantially, there's a good chance the test template will have to be done basically from scratch.

senekor · 2025-01-30T10:28:15Z

exercises/practice/palindrome-products/src/lib.rs

+use std::collections::HashSet;
+
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct Palindrome;


Suggested change

pub struct Palindrome;

pub struct Palindrome {

// TODO

}

While solving exercises, I sometimes wasn't sure what I was supposed to change and what to leave as it is. So I think it could remove some friction to mark this explicitly.

Fair point, added.

senekor · 2025-01-30T10:38:29Z

exercises/practice/palindrome-products/src/lib.rs

-    /// Get the value of this palindrome.
-    pub fn into_inner(self) -> u64 {
-        todo!("return inner value of a Palindrome");
+    pub fn factors(&self) -> &HashSet<(u64, u64)> {


I wonder if we should return on owned hashset here? Having the reference there forces the internal representation of Palindrome to store the hashset. I think that's a good approach, but maybe we shouldn't force it.

Returning an owned hashset might lead to additional heap allocations, but that doesn't matter in my view. If desired, we could do something like fn into_factors(self) -> HashSet, maybe that would yield the most flexibility.

I like the idea, done! 👍

See feedback: exercism#2006 (comment) Co-authored-by: Remo Senekowitsch <[email protected]>

See the feedback: exercism#2006 (comment) Co-authored-by: Remo Senekowitsch <[email protected]>

See feedback: exercism#2006 (comment) Co-authored-by: Remo Senekowitsch <[email protected]>

See the feedback: exercism#2006 (comment) Co-authored-by: Remo Senekowitsch <[email protected]>

senekor · 2025-01-30T11:07:35Z

I'll let you merge yourself so you can decide on the squashed commit message. The default setting is to use the PR description. This is useful, because you can add the [no important files changed] tag at the beginning in the PR description and then you won't forget to add it at merge-time. (Not saying the tag should be added on this PR, it's not applicable.)

ellnix · 2025-01-30T11:17:22Z

I'll let you merge yourself so you can decide on the squashed commit message. The default setting is to use the PR description. This is useful, because you can add the [no important files changed] tag at the beginning in the PR description and then you won't forget to add it at merge-time. (Not saying the tag should be added on this PR, it's not applicable.)

Thanks, but I don't have write access for now. I'm guessing I should wait to be added as a maintainer.

senekor · 2025-01-30T13:27:56Z

Jeremy says he added you, so it should work now 👍

ellnix · 2025-01-30T14:09:33Z

Jeremy says he added you, so it should work now 👍

Github says no:

Maybe I just need to wait.

palindrome-products: require factors in solution

a924024

github-actions bot closed this Jan 30, 2025

ellnix mentioned this pull request Jan 30, 2025

palindrome-products: require factors in solution #2005

Closed

ellnix commented Jan 30, 2025

View reviewed changes

senekor reopened this Jan 30, 2025

senekor reviewed Jan 30, 2025

View reviewed changes

ellnix added a commit to ellnix/exercism-rust that referenced this pull request Jan 30, 2025

palindrome-products: return owned hashset

9512a58

See feedback: exercism#2006 (comment) Co-authored-by: Remo Senekowitsch <[email protected]>

ellnix added a commit to ellnix/exercism-rust that referenced this pull request Jan 30, 2025

palindrome-products: expect struct to be edited

4b11e3d

See the feedback: exercism#2006 (comment) Co-authored-by: Remo Senekowitsch <[email protected]>

ellnix requested a review from senekor January 30, 2025 10:54

ellnix and others added 2 commits January 30, 2025 11:56

palindrome-products: return owned hashset

fe45663

See feedback: exercism#2006 (comment) Co-authored-by: Remo Senekowitsch <[email protected]>

palindrome-products: expect struct to be edited

5dfe332

See the feedback: exercism#2006 (comment) Co-authored-by: Remo Senekowitsch <[email protected]>

ellnix force-pushed the palindrome-products-factors branch from 4b11e3d to 5dfe332 Compare January 30, 2025 10:57

senekor approved these changes Jan 30, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

palindrome-products: require factors in solution #2006

palindrome-products: require factors in solution #2006

ellnix commented Jan 30, 2025

github-actions bot commented Jan 30, 2025

github-actions bot commented Jan 30, 2025

github-actions bot commented Jan 30, 2025

ellnix Jan 30, 2025

senekor Jan 30, 2025

ellnix Jan 30, 2025

senekor commented Jan 30, 2025

ellnix commented Jan 30, 2025

senekor Jan 30, 2025

senekor Jan 30, 2025

ellnix Jan 30, 2025

senekor Jan 30, 2025

ellnix Jan 30, 2025

senekor commented Jan 30, 2025

ellnix commented Jan 30, 2025

senekor commented Jan 30, 2025

ellnix commented Jan 30, 2025 •

edited

Loading

-        {{- test.expected.factors | join(sep=", ") | replace(from="[", to="(") | replace(from="]", to=")") -}}
+        {%- for factor in test.expected.factors -%}
+          ({{ factor | join(sep=", ") }}){% if not loop.last %}, {% endif %}
+        {%- endfor -%}

palindrome-products: require factors in solution #2006

Are you sure you want to change the base?

palindrome-products: require factors in solution #2006

Conversation

ellnix commented Jan 30, 2025

The HashSet

The extra tests

github-actions bot commented Jan 30, 2025

github-actions bot commented Jan 30, 2025

github-actions bot commented Jan 30, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

senekor commented Jan 30, 2025

ellnix commented Jan 30, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

senekor commented Jan 30, 2025

ellnix commented Jan 30, 2025

senekor commented Jan 30, 2025

ellnix commented Jan 30, 2025 • edited Loading

ellnix commented Jan 30, 2025 •

edited

Loading