M-Labs
/
artiq-zynq
10
1
Fork 19
Code Issues 13 Pull Requests Releases Wiki Activity

Create and apply rustfmt policy #221

Merged
sb10q merged 1 commits from esavkin/artiq-zynq:rustfmt-policy into master 2023-02-22 11:02:43 +08:00
Conversation 1 Commits 1 Files Changed 48 +2452 -1728
esavkin commented 2023-02-20 16:33:04 +08:00
Owner
Copy Link

Run cargo fmt as a test after building the firmware.

Proposed to solve such problems #217 (comment).
Cargo fmt can be also integrated to the major IDEs such as VS Code and Clion (see https://github.com/rust-lang/rustfmt#running-rustfmt-from-your-editor)

Code style settings are subject of discuss.

With this change, incorrect formatting will result in such output on running nix develop:

$ nix develop                               
warning: Git tree '/home/esavkin/artiq-zynq' is dirty
Diff in /home/esavkin/artiq-zynq/src/satman/src/repeater.rs at line 280:
         }
 
         if self.state != RepeaterState::Up {
-            return Ok((
-                
-                ))
+            return Ok(())
         }
 
         drtioaux::send(self.auxno, &drtioaux::Packet::ResetRequest).unwrap();
Diff in /home/esavkin/artiq-zynq/src/satman/src/repeater.rs at line 293:
         Ok(())
     }
 }
-
-
-
-
 
 #[cfg(not(has_drtio_routing))]
 #[derive(Clone, Copy, Default)]
Run `cargo fmt` as a test after building the firmware. Proposed to solve such problems https://git.m-labs.hk/M-Labs/artiq-zynq/pulls/217#issuecomment-6558. Cargo fmt can be also integrated to the major IDEs such as VS Code and Clion (see https://github.com/rust-lang/rustfmt#running-rustfmt-from-your-editor) Code style settings are subject of discuss. With this change, incorrect formatting will result in such output on running `nix develop`: ```rust $ nix develop warning: Git tree '/home/esavkin/artiq-zynq' is dirty Diff in /home/esavkin/artiq-zynq/src/satman/src/repeater.rs at line 280: } if self.state != RepeaterState::Up { - return Ok(( - - )) + return Ok(()) } drtioaux::send(self.auxno, &drtioaux::Packet::ResetRequest).unwrap(); Diff in /home/esavkin/artiq-zynq/src/satman/src/repeater.rs at line 293: Ok(()) } } - - - - #[cfg(not(has_drtio_routing))] #[derive(Clone, Copy, Default)] ```
sb10q commented 2023-02-20 16:34:18 +08:00
Owner
Copy Link

Run cargo fmt on nix develop.

A new test on Hydra (flake.nix) is probably better.

> Run cargo fmt on nix develop. A new test on Hydra (flake.nix) is probably better.
👍 1
sb10q reviewed 2023-02-20 16:36:23 +08:00
src/runtime/src/kernel/api.rs Outdated
@ -79,12 +74,10 @@ pub fn resolve(required: &[u8]) -> Option<u32> {
api!(now_mu = rtio::now_mu),
api!(at_mu = rtio::at_mu),
api!(delay_mu = rtio::delay_mu),
sb10q commented 2023-02-20 16:36:23 +08:00
Owner
Copy Link

We want those blank lines.

We want those blank lines.
esavkin commented 2023-02-20 16:46:32 +08:00
Author
Owner
Copy Link

It can be skipped by #[rustfmt::skip] attribute

It can be skipped by `#[rustfmt::skip]` attribute
sb10q commented 2023-02-20 16:47:33 +08:00
Owner
Copy Link

That would skip too much or create noise, no? What about simply using one or two empty comments instead?

That would skip too much or create noise, no? What about simply using one or two empty comments instead?
esavkin commented 2023-02-20 16:49:04 +08:00
Author
Owner
Copy Link

No, it skips only one block, for this case - array only

No, it skips only one block, for this case - array only
sb10q commented 2023-02-20 16:49:21 +08:00
Owner
Copy Link

Ok, sounds good.

Ok, sounds good.
esavkin force-pushed rustfmt-policy from 636fbf9748 to 873f012351 2023-02-20 16:45:32 +08:00 Compare
esavkin force-pushed rustfmt-policy from 873f012351 to 81878a077f 2023-02-21 11:04:54 +08:00 Compare
esavkin changed title from WIP: Create and apply rustfmt policy to Create and apply rustfmt policy 2023-02-21 11:06:07 +08:00
sb10q reviewed 2023-02-21 11:40:20 +08:00
flake.nix Outdated
@ -149,7 +149,10 @@
echo file binary-dist $out/${fwtype}.elf >> $out/nix-support/hydra-build-products
'';
doCheck = false;
sb10q commented 2023-02-21 11:40:20 +08:00
Owner
Copy Link

Separate derivation with just this test is probably better. For example we don't need to run it every time for every variant.

Separate derivation with just this test is probably better. For example we don't need to run it every time for every variant.
esavkin force-pushed rustfmt-policy from 81878a077f to dbf44ddf89 2023-02-21 15:18:44 +08:00 Compare
sb10q reviewed 2023-02-21 18:07:47 +08:00
flake.nix Outdated
@ -256,0 +257,4 @@
name = "fmt_check";
nativeBuildInputs = [
rustPlatform.rust.rustc
sb10q commented 2023-02-21 18:07:47 +08:00
Owner
Copy Link

Do we actually need rustc?

Do we actually need rustc?
sb10q reviewed 2023-02-21 18:08:15 +08:00
flake.nix Outdated
@ -253,6 +253,23 @@
'';
};
fmt_check = pkgs.stdenv.mkDerivation {
sb10q commented 2023-02-21 18:08:15 +08:00
Owner
Copy Link

- as in gateware-sim

``-`` as in ``gateware-sim``
👍 1
sb10q reviewed 2023-02-21 18:09:28 +08:00
flake.nix Outdated
@ -256,0 +265,4 @@
buildPhase =
''
(cd ${self}/src && cargo fmt -- --check)
sb10q commented 2023-02-21 18:09:28 +08:00
Owner
Copy Link

Why this combination? Just write two lines.

Why this combination? Just write two lines.
👍 1
sb10q reviewed 2023-02-21 18:17:16 +08:00
src/libboard_artiq/src/logger.rs Outdated
@ -9,2 +9,2 @@
buffer: MutexGuard<'a, LogBuffer<&'static mut [u8]>>,
old_log_level: LevelFilter
buffer: MutexGuard<'a, LogBuffer<&'static mut [u8]>>,
old_log_level: LevelFilter,
sb10q commented 2023-02-21 18:17:16 +08:00
Owner
Copy Link

It's a bit weird that it would require commas here on the last element, but remove the semicolon on return Err("Neither MCP23017 nor PCA9539 io expander found.") above

It's a bit weird that it would require commas here on the last element, but remove the semicolon on ``return Err("Neither MCP23017 nor PCA9539 io expander found.")`` above
sb10q commented 2023-02-21 18:19:09 +08:00
Owner
Copy Link

Ok, I guess it's because of https://stackoverflow.com/questions/62637801/why-is-the-semicolon-after-the-return-statement-optional

Ok, I guess it's because of https://stackoverflow.com/questions/62637801/why-is-the-semicolon-after-the-return-statement-optional
esavkin commented 2023-02-22 10:17:48 +08:00
Author
Owner
Copy Link

It can be configured. For now I moved back trailing semicolon, but left trailing commas for Vertical items

It can be configured. For now I moved back trailing semicolon, but left trailing commas for `Vertical` items
esavkin force-pushed rustfmt-policy from dbf44ddf89 to fb4c9bb131 2023-02-22 10:10:40 +08:00 Compare
esavkin force-pushed rustfmt-policy from fb4c9bb131 to 938f7e33a7 2023-02-22 10:52:54 +08:00 Compare
sb10q merged commit a519d24074 into master 2023-02-22 11:02:43 +08:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
priority:critical

   
priority:high

   
priority:low

   
priority:medium
No Label
priority:critical
priority:high
priority:low
priority:medium
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: M-Labs/artiq-zynq#221
No description provided.
Delete Branch "esavkin/artiq-zynq:rustfmt-policy"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?