Skip to content
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

Replace GUI exercise with Logger #1682

Merged
merged 4 commits into from
Jan 18, 2024
Merged

Replace GUI exercise with Logger #1682

merged 4 commits into from
Jan 18, 2024

Conversation

djmitche
Copy link
Collaborator

This should be a bit simpler, and notably

But all that hard work developing the GUI exercise is not for naught: it remains in the "Modules" segment, where students will get a chance to read some Rust code and reorganize it a little bit.

Fixes #1617.

R=mgeisler as the original author of the GUI exercise.

@djmitche djmitche requested a review from mgeisler January 10, 2024 22:40
@djmitche
Copy link
Collaborator Author

dprint fmt on my system doesn't make any changes here. Any idea why it's failing in the PR and not locally?

@mgeisler
Copy link
Collaborator

Thanks, I like this a lot! It makes the generics part much more focused and it shows a useful pattern.

dprint fmt on my system doesn't make any changes here. Any idea why it's failing in the PR and not locally?

Not sure! I just tried it here:

% gh pr checkout 1682
remote: Enumerating objects: 17, done.
remote: Counting objects: 100% (17/17), done.
remote: Compressing objects: 100% (7/7), done.
remote: Total 17 (delta 10), reused 16 (delta 10), pack-reused 0
Unpacking objects: 100% (17/17), 3.73 KiB | 954.00 KiB/s, done.
From github.com:google/comprehensive-rust
 * [new ref]         refs/pull/1682/head -> issue1617
Switched to branch 'issue1617'
% dprint fmt
Formatted 1 file.
% git diff                                                           (~/src/comprehensive-rust)
diff --git a/src/modules/exercise.rs b/src/modules/exercise.rs
index 87bf452..09fbb19 100644
--- a/src/modules/exercise.rs
+++ b/src/modules/exercise.rs
@@ -84,8 +84,8 @@ impl Widget for Window {
 
         let inner_width = self.inner_width();
 
-        // TODO: Change draw_into to return Result<(), std::fmt::Error>. Then use the ?-operator
-        // here instead of .unwrap().
+        // TODO: Change draw_into to return Result<(), std::fmt::Error>. Then use the
+        // ?-operator here instead of .unwrap().
         writeln!(buffer, "+-{:-<inner_width$}-+", "").unwrap();
         writeln!(buffer, "| {:^inner_width$} |", &self.title).unwrap();
         writeln!(buffer, "+={:=<inner_width$}=+", "").unwrap();

Ah, I know why!

Wrapping of comments is an unstable rustfmt feature... We therefore run dprint fmt after a rustup default unstable in CI. I also see no output if I switch back to stable temporarily!

Let me try to fix this!

mgeisler added a commit that referenced this pull request Jan 14, 2024
Without this, several of the formatting directives in our
`rustfmt.toml` file won’t have any effect. This will in turn lead to
mismatches between the formatting done locally and in CI.

From the discussion in #1682.
@mgeisler
Copy link
Collaborator

I found a way to run rustfmt from Rust nightly, assuming you have rustup on the system. Please see #1694.

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.

Thanks a lot!

mgeisler added a commit that referenced this pull request Jan 16, 2024
Without this, several of the formatting directives in our `rustfmt.toml`
file won’t have any effect. This will in turn lead to mismatches between
the formatting done locally and in CI.

This creates a dependency on `rustup`, but I think this is overall
better than silently ignoring the formatting directives.

From the discussion in #1682.
@djmitche djmitche enabled auto-merge (squash) January 18, 2024 19:09
@djmitche djmitche merged commit 9d9b417 into google:main Jan 18, 2024
32 checks passed
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.

GUI Exercise is more about text formatting than trait objects
2 participants