MCHPRS icon indicating copy to clipboard operation
MCHPRS copied to clipboard

Remove use of nightly features

Open fee1-dead opened this issue 3 years ago • 6 comments
trafficstars

  • 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.

fee1-dead avatar Sep 18 '22 07:09 fee1-dead

https://github.com/MCHPR/MCHPRS/blob/97743175c8e7e354ee53487607fe0acdb581d03b/crates/core/src/blocks/mod.rs#L30-L59

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.

MalbaCato avatar Sep 18 '22 08:09 MalbaCato

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 {}.

fee1-dead avatar Sep 18 '22 08:09 fee1-dead

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 😅

MalbaCato avatar Sep 18 '22 08:09 MalbaCato

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.

fee1-dead avatar Sep 18 '22 08:09 fee1-dead

Can you quickly update the building part of README?

Olek47 avatar Sep 18 '22 09:09 Olek47

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

fee1-dead avatar Sep 18 '22 09:09 fee1-dead

@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.

fee1-dead avatar Oct 28 '22 12:10 fee1-dead

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

RaitoBezarius avatar Jul 01 '23 11:07 RaitoBezarius