Skip to content

Conversation

@cchiw
Copy link
Collaborator

@cchiw cchiw commented Feb 10, 2023

This code had variables in the function arguments that were not being used (yet).
To address that, in this PR I use the function arguments in a print statement. This changes allow us to remove the line to silence warnings for unused variables #71.

Alternatively, we could comment out the functions. As seen in #389.

@mgeisler
Copy link
Collaborator

Remove flag to silence warnings #71

Please move this to the PR title. The title shows up prominently in the history afterwards so it's helpful when it says more than just "Update foo.md" 🙂

Also, please explain why you make the change. What did you see before and after this PR? That gives me a lot more context as a reviewer.

@cchiw cchiw changed the title Update strings-iterators.md Remove silenced warnings in exercises Feb 13, 2023
@cchiw cchiw changed the title Remove silenced warnings in exercises Remove silenced warnings in string-iterators exercise Feb 13, 2023
@cchiw cchiw requested a review from mgeisler February 13, 2023 19:52
Copy link
Collaborator

@mgeisler mgeisler left a comment

Choose a reason for hiding this comment

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

Doing this makes us lose the coupling with the solution in strings-iterators.rs. So I suggest you put the include back to keep the link.

#![allow(unused_variables, dead_code)]

{{#include strings-iterators.rs:prefix_matches}}
pub fn prefix_matches(prefix: &str, request_path: &str) -> bool {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doing this makes us lose the coupling with the function signature in strings-iterators.rs. Could you put the include back so that we can keep the connection?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, does it make sense to add a new function to the original string-iterators.rs file and refer to it in this file or just add the line back in as is? The commit I just made does the former.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The latter is better: that way we avoid duplication in the solution file and we avoid showing prefix_matches_unimplemented to the students (there is no reason for them to see such a function name).

My goal is to reuse as much as possible from the solution file. We compile the code there in CI so we know it works — when we reuse it, we also have some confidence that the unsolved version is aligned with the solution.

@cchiw cchiw requested a review from mgeisler March 2, 2023 05:09
@cchiw cchiw enabled auto-merge (squash) March 2, 2023 05:10
@cchiw cchiw closed this Mar 6, 2023
auto-merge was automatically disabled March 6, 2023 19:07

Pull request was closed

@cchiw cchiw deleted the cchiw-patch-12 branch March 7, 2023 03:28
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