Closed Antosser closed 2 months ago
Thanks, that's a good idea. I'll take a look when I get a chance.
Can I request to split the PR into two. One for clippy lints and a separate one for the copy feature (which I think is a great idea btw)? If it's not easy for you I can also do it.
In hindsight I should also mention that if you are interested (and you aren't already part of our discord you are welcome to join us on discord if you want to compare your leetcode solutions with others and that kind of thing). There is also a topic (requires being part of the server to access, but it's public) on discord where we do work on this project. You're more than welcome to join us there as well. It's not super active right now but I do intend on changing that in the not to distant future as I think there are a few features that we should add to this project, like setup for new users.
The only clippy specific change was in src/leetcode_env/tree.rs. The others were required to make the copying work. I can revert that file later today
Ok if it's just one then leave it. It's fine no worries.
Will you merge?
Yes I will, just haven't had a chance to review it yet.
Thank you very much for the PR. Really good job following the structure of the code. Looks good overall just have a few questions.
The main one is I haven't seen how the command identifies which file it should copy the text from. It looks like it just uses the first file found that is not lib.rs
. It is quite possible the user has many solutions in the folder and it might copy the wrong one. Please let me know if I missed it. I'm thinking we could either ask for the filename which the shell may be able to auto complete or we could try to use the same format as what we do for generate. Let me know what you think. Alternatively we could use the most recently modified file to make it easier but I'm seeing that might end up being annoying in some cases if another file gets edited for whatever reason.
I made a few other suggestion / added questions attached to specific lines in the code. Let me know if you have trouble finding them.
It looks like it just uses the first file found that is not
lib.rs
.
Yes, that's exactly how I implemented it. I couldn't find it stored anywhere, and this seemed like a good solution.
It is quite possible the user has many solutions in the folder and it might copy the wrong one
Looks like I had a different view of the project. I thought each solution required its own project, which makes more sense to me in Leetcode's case. Having multiple solutions in the same project wouldn't be very practical for the user either, since cargo test, for example, would test all the solutions at once.
It looks like it just uses the first file found that is not
lib.rs
.Yes, that's exactly how I implemented it. I couldn't find it stored anywhere, and this seemed like a good solution.
Yes you are correct it isn't stored anywhere because the application doesn't maintain any state between runs so we'd need the user to supply that information when they run it.
It is quite possible the user has many solutions in the folder and it might copy the wrong one
Looks like I had a different view of the project. I thought each solution required its own project, which makes more sense to me in Leetcode's case. Having multiple solutions in the same project wouldn't be very practical for the user either, since cargo test, for example, would test all the solutions at once.
I understand your point of view however it is setup right now to expect each problem to be a module. This allows for sharing of common resources between problems, like dependencies and specification of rust version. I will make a note to add the assumed project structure to the reademe. If we add support for multiple then I think it will cause each feature we add to require much more effort to work with all structures. We already have two types of file names with and without the problem number.
Some users specify which test they want to run when they call cargo test
but I actually only keep the modules in lib.rs
that I am currently working on so only those get run / compiled.
Updating with my current thoughts after the discussion in Issue https://github.com/rust-practice/cargo-leet/issues/86
I think the fundamental problem is that we have a different perspective on keeping already submitted solutions. At first I was comparing the two options in the context of keeping solutions. But given that you do not intend to do that, yours is just a subset of mine. It's the case of a new setup with the first solution being done. So anything that works for my setup should also work for yours. I've given the cache setup more thought. It only causes problems if it's global. Local variables to the rescue. I thought about how it's done in the mainmatter workshop runner and they use a database in the same folder to store state but I'm not familiar with using Sqlite yet. If I thought it would be a better solution I'd invest the time now but I think just a state struct that we persist to disk in ron or json format would also work and we just add it to the .gitignore
so it doesn't get committed. The struct could look like the following:
pub struct AppState {
pub last_problem_slug: Option<String>,
pub should_use_problem_number_in_name: bool,
}
Don't stress about the implementation complexity. I don't mind implementing it if desired. I'm thinking that we can load the state on app startup and persist it to disk on successful exit. Does this approach solve the problem in a way that works for you?
@Antosser how do you feel about this approach?
Sorry for the late response! I appreciate your thoughts and suggestions in #86.
I still have some thoughts about the design choice of having multiple solutions in a single crate. For me, it's similar to why I prefer keeping my other projects in separate crates. This structure allows me to avoid specifying the exact project to build, making things simpler when I have that project in my working directory.
This approach means I can just run cargo test
instead of something like cargo test -p container-with-most-water -- container_with_most_water::tests
(unless there's a shorter way I’m missing?).
I see the value in your suggestion of using a cache to track the current solution, but it feels a bit like reimplementing what working directories (cd
) already achieve. I understand that some prefer having all their solutions in one directory, but I still find the fundamental design decision a bit challenging.
Thanks again for your insights and willingness to implement the solution. Does this perspective make sense from your end?
Sorry for the late response! I appreciate your thoughts and suggestions in #86.
No worries let not your heart be troubled.
I still have some thoughts about the design choice of having multiple solutions in a single crate. For me, it's similar to why I prefer keeping my other projects in separate crates. This structure allows me to avoid specifying the exact project to build, making things simpler when I have that project in my working directory.
Technically we could achieve that using workspaces to be able to just cd
into the directory and have only one project there but I think that swings too far the other way in terms of number of files stored. Furthermore I don't plan to change the structure I'm currently using so separate crates isn't a solution that works for me.
Right now I don't have to specify which one to run. I just use cargo test
I only keep the one I'm working on in lib.rs as a module. I just keep the others there so I can search over them and refer to them as desired.
This approach means I can just run
cargo test
instead of something likecargo test -p container-with-most-water -- container_with_most_water::tests
(unless there's a shorter way I’m missing?).
There is and isn't. If you look at mine you can see that lib.rs is basically empty so yes when I'm using it I use it just like you in terms of doing cargo test
.
I see the value in your suggestion of using a cache to track the current solution, but it feels a bit like reimplementing what working directories (
cd
) already achieve. I understand that some prefer having all their solutions in one directory, but I still find the fundamental design decision a bit challenging.
Since it's not a new project it would mean changing an old decision and changing it means that it no longer works the way I use it....
Thanks again for your insights and willingness to implement the solution. Does this perspective make sense from your end?
Yes I follow your perspective but unfortunately I think changing the overall design to only support one file is a non-starter. We'd need to come up with a solution that works for both because I do keep my old files and don't want to suddenly not be able to use it anymore.
Looks like a support legacy code at all vs. technically more correct, and I don't think I can expect that core design principle to change any time soon
I disagree that your approach is better. Just because you choose not to keep your completed exercises does not make your approach better. It is not legacy code... It's the code in use and is working as designed. You are correct in your assessment of changes to core principles. They will not be changed without sufficient reason. The reason provided thus far does not warrant breaking backward compatibility.
There doesn't seem to be interest in moving this forward at this time. May revisit in the future.
After thinking about it some more, I do see both the ups and downs, and at least for this repo, which already has active users, my approach doesn't make sense. I strove for absolute correctness in the way you choose your current problem you're working on, and mine had some other downs I didn't see. In any way, I still really like the project and look forward to contributing, keeping that in mind!
Thanks, looking forward to working with you. If you want to revisit this let me know and I'll try to do any parts that are giving you issues.
This PR mainly add the copy subcommand in the
src/tool/core/copy.rs
file. Other files also had to be modified. The subcommand automatically copies the leetcode-relevant code, so it can be pasted directly into leetcode