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

Remove use of nightly features #86

Merged
merged 3 commits into from
Jul 1, 2023

Conversation

fee1-dead
Copy link
Contributor

  • Removed dependency on regex's pattern feature: find_iter already does what is needed.
  • Replacing a specialized default blanket impl with a macro that generates the impl (so types need to be manually added to implement BlockTransform) This is more correct because when new blocks are added that need to implement BlockTransform, someone could forget to implement and it will just fallback to the blanket impl instead (previous behavior) After this change any new types do not get BlockTransform automatically and an impl must be added manually.
  • Replaced the std once_cell feature with the once_cell crate. As shown in Cargo.lock, this is already one of the transitive dependencies, so adding this as a dependency will not introduce more bloat.

@MalbaCato
Copy link

trait BlockTransform {
fn rotate(&mut self, amt: crate::blocks::RotateAmt) {
match amt {
// ez
RotateAmt::Rotate90 => self.rotate90(),
RotateAmt::Rotate180 => {
self.rotate90();
self.rotate90();
}
RotateAmt::Rotate270 => {
self.rotate90();
self.rotate90();
self.rotate90();
}
}
}
fn rotate90(&mut self);
fn flip(&mut self, dir: crate::blocks::FlipDirection);
}
macro_rules! noop_block_transform {
($($ty:ty),*$(,)?) => {
$(
impl BlockTransform for $ty {
fn rotate90(&mut self) {}
fn flip(&mut self, dir: crate::blocks::FlipDirection) {}
}
)*
};
}

Isn't this better as

 trait BlockTransform { 
     fn rotate(&mut self, amt: crate::blocks::RotateAmt) { 
         // ...
     } 
     fn rotate90(&mut self) {}; 
     fn flip(&mut self, dir: crate::blocks::FlipDirection) {}; 
 } 
  
 macro_rules! noop_block_transform { 
     ($($ty:ty),*$(,)?) => { 
         $( 
             impl BlockTransform for $ty; 
         )* 
     }; 
 }

? it would also allow easily implementing blocks that do rotate, but who's flip is a no-op, for example.

@fee1-dead
Copy link
Contributor Author

This is just a design choice that the person who wrote this did not choose to have default no-op bodies. AFAIK you can't do impl Tr for Ty; and you can only do impl Tr for Ty {}.

@MalbaCato
Copy link

well, is it a good design choice? considering that's basically what the macro (previously blanket impl) is doing?
and yes, probably should have code that compiles 😅

@fee1-dead
Copy link
Contributor Author

fee1-dead commented Sep 18, 2022

Also the fact that the code compiles after this change shows that manual impls all have both flip and rotate90. So having default bodies is of dubious utility unless there is evidence that this is desired.

@Olek47
Copy link
Contributor

Olek47 commented Sep 18, 2022

Can you quickly update the building part of README?

@fee1-dead
Copy link
Contributor Author

Done. I haven't touched the CI configs and Docker stuff.

@fee1-dead
Copy link
Contributor Author

@StackDoubleFlow is this something that is desired? Requiring nightly would make it harder to package/build on linux distributions. Also if a user uses distribution provided rust then they can only use stable.

@RaitoBezarius
Copy link

Friendly ping, we are trying to package MCHPRS in Nixpkgs and nightly features makes it harder to package it.

@StackDoubleFlow StackDoubleFlow merged commit 76428a8 into MCHPR:master Jul 1, 2023
@fee1-dead fee1-dead deleted the rm-nightly branch September 23, 2024 10:41
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.

5 participants