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

feat - make order that readout keys are listed in macchina.toml affec… #275

Merged
merged 1 commit into from
Mar 1, 2023

Conversation

Rolv-Apneseth
Copy link
Contributor

…t the order readouts are displayed in.

Also minor change to the documentation to note that the order now matters.

Hope this is alright but of course don't hold back with criticisms / suggestions.

I'm still new to rust and wasn't sure how / if I should split up this file so I left everything in the one file but perhaps it's too long. Also I realise that this change makes the order have to matter rather than it being optional, if you would like it to be optional I can look into it, maybe add another config option for show_order_matters = "true".

Thanks for making this tool and thanks in advance for any feedback, I'm new to rust so any tips you have are appreciated.

@grtcdr
Copy link
Member

grtcdr commented Feb 27, 2023

That was quick!

Hope this is alright but of course don't hold back with criticisms / suggestions.

I went over the changes and they look fine to me, the one function per
readout pattern seems like our only way out, although we could benefit
from defining a macro to handle some of the boilerplate.

I'm still new to rust and wasn't sure how / if I should split up this
file so I left everything in the one file but perhaps it's too
long.

Good call, splitting them up even further doesn't seem all that
necessary.

Also I realise that this change makes the order have to matter
rather than it being optional, if you would like it to be optional I
can look into it, maybe add another config option for
show_order_matters = "true".

I think users will intuitively assume that the order in which
readouts appear matters and prefer it to keep it that way. If someone
doesn't care for the order, show_order_matters already doesn't
matter to them.

Thanks for making this tool and thanks in advance for any feedback,
I'm new to rust so any tips you have are appreciated.

Thank you for your help!

@Rolv-Apneseth
Copy link
Contributor Author

I went over the changes and they look fine to me, the one function per
readout pattern seems like our only way out, although we could benefit
from defining a macro to handle some of the boilerplate.

Any idea how I could try to implement this? Macros are one thing I haven't really learned yet but what should it to do to help cut down on boilerplate?

@grtcdr
Copy link
Member

grtcdr commented Feb 27, 2023

I'm not really sure yet, I haven't had much experience using Rust's macro system.

EDIT: It seems the tiny variations between the different handlers would make the macro in question really complicated to implement.

@Rolv-Apneseth
Copy link
Contributor Author

If the macro would be to complicated to implement, is there anything else you'd like from this pr or is that alright as is?

@grtcdr
Copy link
Member

grtcdr commented Mar 1, 2023

If the macro would be to complicated to implement, is there anything else you'd like from this pr or is that alright as is?

Nope, this is good enough for now.

Thank you for helping improve macchina :)

@grtcdr grtcdr merged commit ee21fcd into Macchina-CLI:main Mar 1, 2023
@Rolv-Apneseth Rolv-Apneseth deleted the customise-readouts-order branch September 29, 2024 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants