Skip to content
This repository has been archived by the owner on Sep 20, 2024. It is now read-only.

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 subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants