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

fix the issue of not reading too long file. #4

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

LoserFox
Copy link

longer than 65565 bytes content will not be processed.

@LoserFox
Copy link
Author

although a bit offtopic, but i recommend (?m)(?:text = "|[^s]tr\(["'])(.*?)(?:"|["']\)) change to (?m)(?: = "|[^s]tr\(["'])(.*?)(?:"|["']\)) to get custom export variable support.

Copy link
Owner

@mrombout mrombout left a comment

Choose a reason for hiding this comment

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

I tried extracting from a 709585 bytes on a build without this patch and it extracted without problem. But a streaming approach is indeed better than loading the entire file into memory as was done previously.

return poFile, err
f, e := os.Open(filePath)
if e != nil {
panic(e)
Copy link
Owner

Choose a reason for hiding this comment

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

I think it's better to keep the panic()s in the main and use plain errors throughout the code base.

Suggested change
panic(e)
return poFile, err

content, err := fs.readFile(filePath)
if err != nil {
return poFile, err
f, e := os.Open(filePath)
Copy link
Owner

Choose a reason for hiding this comment

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

Revert to err to keep in line with standard go practices.

Suggested change
f, e := os.Open(filePath)
f, err := os.Open(filePath)

content, err := fs.readFile(filePath)
if err != nil {
return poFile, err
f, e := os.Open(filePath)
Copy link
Owner

Choose a reason for hiding this comment

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

In order to keep the tests working, this os.Open call needs to go through the fileSystem interface and then be mocked.

Or better yet, these days it's probably better to rely on fs.FS.

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

2 participants