search

Shizcow / dmenu-rs

A pixel perfect port of dmenu, rewritten in Rust with extensive plugin support
GNU General Public License v3.0
197 stars 9 forks source link

port stest to rust #47

Closed benjaminedwardwebb closed 1 year ago

benjaminedwardwebb commented 1 year ago

I took the opportunity to learn some rust (something I'd been meaning to do) and port stest over from C, as suggested in this issue. Hopefully the rust looks reasonable, but open to any advice and tips of course.

Since stest was the only remaining C artifact, this pull request also drops all references to the C toolchain from the makefile and build. Yay, less C and simpler build!

verification

I wrote some integration tests for stest. They're not perfect but they cover a lot of the basic cases.

I also ran modified versions of dmenu_path and dmenu_run that pointed to the rust-based stest built locally on my x86_64 GNU/Linux host, and dmenu ran as expected.

benjaminedwardwebb commented 1 year ago

Sorry for just now getting to this.

Not a problem at all. You're certainly under no obligation to maintain open-source projects if you're otherwise busy.

I appreciate the suggestions and comments you've made. I've responded to some of the simpler comments, and am working on addressing the issues you've called out regarding error-handling and the checkdeps.sh script. I should have time tomorrow to sort through some of it.

Have a good weekend. :)

Shizcow commented 1 year ago

I appreciate the suggestions and comments you've made. I've responded to some of the simpler comments, and am working on addressing the issues you've called out regarding error-handling and the checkdeps.sh script. I should have time tomorrow to sort through some of it.

Not a problem. If you can't/don't want to fix the checkdeps/other issues I've mentioned I can do that in a week or two myself. Looking forward to getting this merged :smile:

benjaminedwardwebb commented 1 year ago

I appreciate the suggestions and comments you've made. I've responded to some of the simpler comments, and am working on addressing the issues you've called out regarding error-handling and the checkdeps.sh script. I should have time tomorrow to sort through some of it.

Not a problem. If you can't/don't want to fix the checkdeps/other issues I've mentioned I can do that in a week or two myself. Looking forward to getting this merged smile

I ended up not having the time I thought I would last weekend to finish this. I won't be able to pick it up until next week some time, maybe next weekend.

Please feel free to merge anything else in the mean time, I'm happy to rebase over any changes. Alternatively, if you want to merge sooner you're welcome to merge it as-is and I can continue to work on the other pieces in a follow-on.

benjaminedwardwebb commented 1 year ago

@Shizcow I think I've addressed your main concerns now, including the checkdeps.sh/$CC issue.

Anything holding this PR from being merged?

Shizcow commented 1 year ago

@benjaminedwardwebb Everything looks great. There are a few followup items that need addressed (checkdeps.sh is still broken and will cause build to fail on some systems), but I'll be able to fix these today after merging.

Thank you SO much for your work on this. This is a great contribution that makes a measurable impact on this project. Let me know if you need anything from me in the future (including other NIX-related work).