Skip to content

fix issue 80 #81

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

Merged
merged 15 commits into from
Aug 14, 2022
Merged

fix issue 80 #81

merged 15 commits into from
Aug 14, 2022

Conversation

wendajiang
Copy link
Collaborator

@wendajiang wendajiang commented Jul 12, 2022

#80

  1. use code.test config to open/close writing test file
  2. use code.comment_leading to comment the desc
  3. use code.comment_problem_desc to open/close the description about the problem
  4. use code.start_marker to mark the code begin
  5. use code.end_marker to mark the code end

But, this update need user to update the toml config, otherwise the program get panic error massage, I not get the perfect method to handle this. Do you have a good way to handle this?
@bulletmark @clearloop

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
1. use code.test config to open/close writing test file
2. use code.comment_leading to comment the desc
3. use code.comment_problem_desc to open/close the description about the problem
4. use code.start_marker to mark the code begin
5. use code.end_marker to mark the code end
@wendajiang
Copy link
Collaborator Author

const LANGS = [
  {lang: 'bash',       ext: '.sh',         style: '#'},
  {lang: 'c',          ext: '.c',          style: 'c'},
  {lang: 'cpp',        ext: '.cpp',        style: 'c'},
  {lang: 'csharp',     ext: '.cs',         style: 'c'},
  {lang: 'golang',     ext: '.go',         style: 'c'},
  {lang: 'java',       ext: '.java',       style: 'c'},
  {lang: 'javascript', ext: '.js',         style: 'c'},
  {lang: 'kotlin',     ext: '.kt',         style: 'c'},
  {lang: 'mysql',      ext: '.sql',        style: '--'},
  {lang: 'php',        ext: '.php',        style: 'c'},
  {lang: 'python',     ext: '.py',         style: '#'},
  {lang: 'python3',    ext: '.py',         style: '#'},
  {lang: 'ruby',       ext: '.rb',         style: '#'},
  {lang: 'rust',       ext: '.rs',         style: 'c'},
  {lang: 'scala',      ext: '.scala',      style: 'c'},
  {lang: 'swift',      ext: '.swift',      style: 'c'},
  {lang: 'typescript', ext: '.ts',         style: 'c'}
];

As now I introduce the code.comment_leading to let user to judge the comment style, should we use the default config like this?

@wendajiang wendajiang marked this pull request as ready for review July 12, 2022 16:19
@bulletmark
Copy link

I just ran this but clearly something is wrong. I have not tried anything more.

$ ./leetcode  l
error: missing field `start_marker` for key `code` at line 56 column 1, Parse config file failed,
leetcode-cli has just generated a new leetcode.toml at ~/.leetcode/leetcode_tmp.toml,
the current one at ~/.leetcode/leetcode.tomlseems missing some keys, please compare to the
tmp one and add them up : )

$ ls -l ~/.leetcode
total 4
-rw-r--r-- 1 mark mark 2139 Jul 12 14:27 leetcode.toml

Also, please fix the English:

Parse config file failed. leetcode-cli has just generated a new configuration file at ~/.leetcode/leetcode_tmp.toml as the
existing file ~/.leetcode/leetcode.toml is missing keys. Please compare the new file and add the missing keys.

Why can't you list out the missing keys in the error message?

@wendajiang
Copy link
Collaborator Author

I just ran this but clearly something is wrong. I have not tried anything more.

$ ./leetcode  l
error: missing field `start_marker` for key `code` at line 56 column 1, Parse config file failed,
leetcode-cli has just generated a new leetcode.toml at ~/.leetcode/leetcode_tmp.toml,
the current one at ~/.leetcode/leetcode.tomlseems missing some keys, please compare to the
tmp one and add them up : )

$ ls -l ~/.leetcode
total 4
-rw-r--r-- 1 mark mark 2139 Jul 12 14:27 leetcode.toml

Also, please fix the English:

Parse config file failed. leetcode-cli has just generated a new configuration file at ~/.leetcode/leetcode_tmp.toml as the existing file ~/.leetcode/leetcode.toml is missing keys. Please compare the new file and add the missing keys.

Why can't you list out the missing keys in the error message?

Yeah, as i say, I know this problem, so if you have some suggestions about User must update their leetcode.toml ?

Or, I can fix this generate a leetcode_tmp.toml file( it should a old bug)

@bulletmark
Copy link

The problem is that your error message says a ~/.leetcode/leetcode_tmp.toml file is created but (as I show above!) there is no such file. So when you say "you know the problem", is that what you are referring to?

@wendajiang
Copy link
Collaborator Author

The problem is that your error message says a ~/.leetcode/leetcode_tmp.toml file is created but (as I show above!) there is no such file. So when you say "you know the problem", is that what you are referring to?

I know is not be created and maybe an old bug. My question is if it is the best way to help user update their toml config.

@bulletmark
Copy link

I've never had leetcode say to me that it has generated a new leetcode.toml at ~/.leetcode/leetcode_tmp.toml. So that must be a new change and thus if the file is missing that must be a new bug.

@wendajiang
Copy link
Collaborator Author

Please compare the new file and add the missing keys.

The newest version of this branch can generate the leetcode_tmp.toml file, you can try it. And I think this PR is ready to merge. However, If you give more suggestion, I will do more work to improve the experience. @clearloop @bulletmark

@bulletmark
Copy link

That's better. It's a pity that the user had to change the "comment_leading" value every time he changes "lang".

  1. You have added a option to turn "comment_problem_desc" which is good but the user should also have a way to turn the start and end markers off so they are not added at all. I tried setting them to an empty string but that did not work.

  2. I thought your original idea to add a -t option to the edit command was a better idea than how it worked before but I see you got rid of that and gone back to the original approach, although you have given an option to disable test files. Why not simply give the user the option to add the file with edit -t? That is a much better approach then having to edit the config file to turn it on and off.

  3. The title of this PR is "Fix issue 80" but if you look at my issue Flaws in version 0.3.12 #80 then you have not addressed items number 4 and 5.

  4. As a note to both you and @clearloop , the leetcode.toml file unfortunately contains constants (at the top) which are not relevant to the user and clutter/confuse the file (i.e. everything in [sys] and [sys.urls]). In fact if the user edits those then he will break things. So that stuff should not be in a user configuration file or directory. Embed them in/with the program.

@bulletmark
Copy link

bulletmark commented Jul 14, 2022

Another issue:

You can't test or submit any of your old solutions (or new solutions) unless they have the @lc code=start/end markers present. Otherwise you just get an error. The logic should only filter the file only if the markers are present. Note that this fix must be independent of the fix above needed to enable/disable the inclusion of markers when the file is created.

@bulletmark
Copy link

And another issue:

I can't work out what pattern causes this but some times the lines before @lc code=start are not filtered out or are filtered out in a partial way because I get "Runtime Error" for test or submit commands. If I paste the solution to leetcode directly it works, or if I hand comment out all the lines before the marker then it works also. This works for some problems and not for others, even problems that have essentially the same code. Just can't work out what different pattern causes it.

It should be simple logic:

  1. If the solution file has a @lc code=start marker then discard that marker line and all previous lines when sending.
  2. If the solution file has a @lc code=end marker then discard that line and all following lines when sending.
  3. There may be one marker line only in the file (either one of above) but above 2 rules still apply. I.e. user may have a "start" marker and no "end" marker, or vice-versa but still should filter. By default "start" is at start of file and "end" is at end of file.
  4. There may be no marker lines and above 2 rules still apply (i.e. all lines are sent).
  5. Should be irrelevant what comment characters are used on the marker lines. Just look for the marker pattern anywhere on a line.

@wendajiang
Copy link
Collaborator Author

wendajiang commented Jul 14, 2022

And another issue:

I can't work out what pattern causes this but some times the lines before @lc code=start are not filtered out or are filtered out in a partial way because I get "Runtime Error" for test or submit commands. If I paste the solution to leetcode directly it works, or if I hand comment out all the lines before the marker then it works also. This works for some problems and not for others, even problems that have essentially the same code. Just can't work out what different pattern causes it.

Do you give a failed exmpale?

It should be simple logic:

  1. If the solution file has a @lc code=start marker then discard that marker line and all previous lines when sending.
  2. If the solution file has a @lc code=end marker then discard that line and all following lines when sending.
  3. There may be one marker line only in the file (either one of above) but above 2 rules still apply. I.e. user may have a "start" marker and no "end" marker, or vice-versa but still should filter. By default "start" is at start of file and "end" is at end of file.
  4. There may be no marker lines and above 2 rules still apply (i.e. all lines are sent).
  5. Should be irrelevant what comment characters are used on the marker lines. Just look for the marker pattern anywhere on a line.

The rule is simple that the content betwteen the end offset of @lc code=start and the start offset of @lc code=end would be send for testing or submitting.

Maybe should add a config item turn on/off the marker feature, but I think start and end marker should be on or off meanwhile. So It's not possible there is only @lc code=start or @lc code=end marker.

@wendajiang
Copy link
Collaborator Author

wendajiang commented Jul 14, 2022

That's better. It's a pity that the user had to change the "comment_leading" value every time he changes "lang".

  1. You have added a option to turn "comment_problem_desc" which is good but the user should also have a way to turn the start and end markers off so they are not added at all. I tried setting them to an empty string but that did not work.

Yeah, maybe should add a config item that control the marker feature.
And I propose:

  1. edit_desc to control the desc about problem
  2. If can not find the start marker, send all content to test or submit
  1. I thought your original idea to add a -t option to the edit command was a better idea than how it worked before but I see you got rid of that and gone back to the original approach, although you have given an option to disable test files. Why not simply give the user the option to add the file with edit -t? That is a much better approach then having to edit the config file to turn it on and off.

About this, I think user maybe think every input the command should edit -t a little fussy, and this behavior should be fixed, so I add to the configuration file not a option in the command.

  1. The title of this PR is "Fix issue 80" but if you look at my issue Flaws in version 0.3.12 #80 then you have not addressed items number 4 and 5.
  2. As a note to both you and @clearloop , the leetcode.toml file unfortunately contains constants (at the top) which are not relevant to the user and clutter/confuse the file (i.e. everything in [sys] and [sys.urls]). In fact if the user edits those then he will break things. So that stuff should not be in a user configuration file or directory. Embed them in/with the program.

Maybe the last two question, @clearloop can do better response, after all I only propose some PR for this project.

@clearloop clearloop merged commit 7ea48b8 into clearloop:master Aug 14, 2022
@wendajiang wendajiang mentioned this pull request Sep 23, 2022
Congee pushed a commit to Congee/leetcode-cli that referenced this pull request Oct 13, 2022
* fix: clippy warning for rust 1.61 stable

* feat: upgrade dep version, and use Rust 2021 Edition

* fix: issue 80

1. use code.test config to open/close writing test file
2. use code.comment_leading to comment the desc
3. use code.comment_problem_desc to open/close the description about the problem
4. use code.start_marker to mark the code begin
5. use code.end_marker to mark the code end

* fix: create empty test file

* revert: use superscript and subscript, not ^ and _

* fix: not generate leetcode_tmp.toml

* fix: clippy warning

* chore: upgrade dep

* backwark compability

* cfg marker trun on/off
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.

None yet

3 participants