danyspin97 / wpaperd

Modern wallpaper daemon for Wayland
GNU General Public License v3.0
332 stars 26 forks source link

Add shell expansion #11

Closed 1995parham closed 2 years ago

1995parham commented 2 years ago

Closes #9

Based on shellexpand crate.

1995parham commented 2 years ago

@danyspin97 Thanks for this awesome tool. I am new to rust, so if there is any issue with this PR I will be happy to fix.

danyspin97 commented 2 years ago

Hi Parham and thank you for the contribution. I have looked up for shellexpand and it's not maintained anymore, so I'd prefer to avoid using it. I'd say that supporting the tilde expansion should be enough and we can manually expand it.

1995parham commented 2 years ago

Sure @danyspin97 you are right. I will change it to manually expand ~.

1995parham commented 2 years ago

I used dirs package, which is a dependency of xdg package, for finding the home and replacing the tilde with it. We can have more replace too if you want.

1995parham commented 2 years ago

@danyspin97 Sorry for bothering you, what do you think about this PR? Replacing ~ is enough?

danyspin97 commented 2 years ago

Replacing ~ is enough?

Yea, I think it is enough. However, there are still a couple of issues that I have highlighted in the comments. Can you please have a look? Also, this function should also be applied to clap --config- parameter, but I'll implement this after this PR gets merged.

1995parham commented 2 years ago

@danyspin97 Thanks. I can't see those comments, can you please mention me there? I have only one comment about not using the shellexpand package, which I fixed.

danyspin97 commented 2 years ago

Can you see them if you go to "Files changed" tab in this PR?

1995parham commented 2 years ago

I don't see anything there, I think you should complete your review, so I can see your comments.

image

danyspin97 commented 2 years ago

Sorry, I hadn't actually sent the review. Fixed now 😄