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

Add copy subcommand & fix clippy warnings #84

Closed
wants to merge 6 commits into from

Conversation

Antosser
Copy link
Contributor

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

@c-git
Copy link
Contributor

c-git commented Jun 12, 2024

Thanks, that's a good idea. I'll take a look when I get a chance.

@c-git
Copy link
Contributor

c-git commented Jun 12, 2024

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.

@c-git
Copy link
Contributor

c-git commented Jun 12, 2024

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.

@Antosser
Copy link
Contributor Author

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

@c-git
Copy link
Contributor

c-git commented Jun 13, 2024

Ok if it's just one then leave it. It's fine no worries.

@Antosser
Copy link
Contributor Author

Will you merge?

@c-git
Copy link
Contributor

c-git commented Jun 17, 2024

Yes I will, just haven't had a chance to review it yet.

@c-git
Copy link
Contributor

c-git commented Jun 18, 2024

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.

@Antosser
Copy link
Contributor Author

Antosser commented Jun 18, 2024

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.

@c-git
Copy link
Contributor

c-git commented Jun 20, 2024

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.

// Logging
info!("Starting copy function");

// Find the first file in src/ that is not lib.rs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the discussion in the conversation of this PR we need to decide how we want to handle this. One other other option I was thinking about is we have a config file in the user's home directory which tracks the most recent problems they worked on and we could get the most recent one from there. I can help with implementation if this seems daunting.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That said the easiest option is to have the user specify the problem but I'm concerned about the UX of that option. I feel auto complete may actually make specifying the file not that hard but they would have to think about what the problem number or name is and this might just end up being harder than just doing the copy and paste themselves.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolve this in favor of #86?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, Issue #86 is a reminder to document the current structure it doesn't solve our decision regarding how we want to implement this feature. I think we have a few choices to pick from (you're also welcome to add any I may have missed):

  1. Ask the user for the filename
  2. Ask the user for the problem and determine the filename from that
  3. Add the capability to the tool to save setting and track the problems that they recently started

@@ -61,6 +61,8 @@ impl Cli {
pub enum Commands {
#[clap(visible_alias = "gen", short_flag = 'g')]
Generate(GenerateArgs),
/// Copies the code from ??? to your clipboard for pasting on leetcode
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for confusing you, I meant that we need to document it and the question marks need to be replaced once we decide how we want to handle picking which file to copy from.

@c-git
Copy link
Contributor

c-git commented Jun 21, 2024

Updating with my current thoughts after the discussion in Issue #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?

@c-git
Copy link
Contributor

c-git commented Jul 3, 2024

@Antosser how do you feel about this approach?

@Antosser
Copy link
Contributor Author

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?

@c-git
Copy link
Contributor

c-git commented Jul 22, 2024

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 like cargo 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.

@Antosser
Copy link
Contributor Author

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

@c-git
Copy link
Contributor

c-git commented Jul 23, 2024

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.

@c-git
Copy link
Contributor

c-git commented Aug 23, 2024

There doesn't seem to be interest in moving this forward at this time. May revisit in the future.

@c-git c-git closed this Aug 23, 2024
@Antosser
Copy link
Contributor Author

Antosser commented Aug 23, 2024

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!

@c-git
Copy link
Contributor

c-git commented Aug 23, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants