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

bug: Large chunks of code missing with remove comments on #55

Closed
KrunchMuffin opened this issue Aug 21, 2024 · 14 comments
Closed

bug: Large chunks of code missing with remove comments on #55

KrunchMuffin opened this issue Aug 21, 2024 · 14 comments
Labels
bug Something isn't working

Comments

@KrunchMuffin
Copy link

this was happening before update to 0.1.27.

I don't really want to post the code here, so if you want to message me or something, then I can send you the files.

@yamadashy
Copy link
Owner

Thank you for reporting this! I'll check it out as soon as I get home.
Your feedback is greatly appreciated!

@yamadashy
Copy link
Owner

@KrunchMuffin
Hi,

Thank you for reporting this issue. To help us investigate and resolve the problem, it would be very helpful if you could share the Python file you mentioned.

If you don't mind, could you please send the file to me at koukun0120@gmail.com?

Thank you again for your help in improving Repopack.

@yamadashy
Copy link
Owner

It appears that f-strings are being inadvertently removed. I'll work on a fix for this issue.
Thank you for bringing this to our attention.

@yamadashy
Copy link
Owner

I've simplified our approach:

  • Now we only remove single-line comments starting with '#'.
  • All other content, including string literals, is preserved as-is.
  • We've decided not to handle docstrings for now due to complexity.

This should fix the issue with f-strings. Let us know if you see any other problems.

@yamadashy
Copy link
Owner

yamadashy commented Aug 24, 2024

This issue has been addressed in v0.1.30. You can find the release notes and update information here:
https://github.com/yamadashy/repopack/releases/tag/v0.1.30

Thank you for your report and patience!

@KrunchMuffin
Copy link
Author

KrunchMuffin commented Aug 24, 2024 via email

@yamadashy
Copy link
Owner

Certainly! I'll check out your PR for docstrings. Thank you for your contribution.

@thecurz
Copy link
Contributor

thecurz commented Aug 30, 2024

With the current regex, there seems to be a similar issue in Python where the comment removal algorithm removes part of a string if it has a # in it.

Example (generated with v0.1.31):
Input:
image
Output:
image

I'm currently trying to fix this and the docstring issue in thecurz/repopack@56f88a3 and I also added extra tests in thecurz/repopack@4abd583.

It might be better to use a native tool like the tokenize module in Python as mentioned here. However, that would mean adding Python to the stack.

I'd appreciate your thoughts on whether I should:

  1. Keep trying to implement an algorithm in TypeScript.
  2. Use Python's native tokenize module, despite having to add a Python script.
  3. Leave this feature as is to avoid more complexity.

Thanks!

@yamadashy
Copy link
Owner

@thecurz
Hi,
Thanks for reporting this issue and working on a fix! I appreciate your detailed analysis.

I agree with your assessment about the tokenizer. For now, let's proceed without adding a Python script or implementing a complex tokenizer. Your current approach in TypeScript seems like the best balance between solving the issue and maintaining the project's simplicity.

I haven't thoroughly reviewed your changes yet, but I wanted to acknowledge your contribution.
I'll examine the fixes in thecurz/repopack@56f88a3 and the tests in thecurz/repopack@4abd583 soon. If you haven't already, could you create a pull request with your changes?

Thank you again for your contribution to improving Repopack!

@thecurz
Copy link
Contributor

thecurz commented Aug 31, 2024

@yamadashy
Thanks for replying! As of now the tests you added on KrunchMuffin's fork are passing on my branch. However, I added extra tests with edge cases to make sure this solution doesn't generate even more issues.

I just wanted to hear your opinion before proceeding with this solution. Once I get all tests passing I'll get back to you on a PR. Cheers!

@KrunchMuffin
Copy link
Author

@thecurz Where you able to get it working well?

@thecurz
Copy link
Contributor

thecurz commented Sep 18, 2024

@KrunchMuffin I'm sorry for the massive delay, I just submitted a pull request solving this.

@yamadashy
Copy link
Owner

yamadashy commented Sep 22, 2024

Thank you, @thecurz and @KrunchMuffin, for your contributions to this issue!

I have just merged the pull request

And released version 0.1.37, which addresses the problem.
https://github.com/yamadashy/repopack/releases/tag/v0.1.37

Thank you again for your effort and contribution! I truly appreciate you taking the time to help with this project.

@yamadashy yamadashy added the bug Something isn't working label Nov 6, 2024
@yamadashy
Copy link
Owner

Since there have been no new reports after the release of v0.1.37 and the feature appears to be working properly, I'll close this issue.

Thank you for reporting and helping resolve this issue!

If you encounter similar issues in the future, please feel free to open a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants