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: add error message for file not found #227

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Edythator
Copy link

Add an error message when trying to locate a non-existing file inside an archive when using extract-file.

Add an error message when trying to locate a non-existing file
inside an archive when using `extract-file`.
@Edythator Edythator changed the title Add error message for file not found fix: add error message for file not found Jan 24, 2022
@substanc3-dev
Copy link

Tests seem to be failing, have you looked into what might be the cause of that?

@erickzhao
Copy link
Member

Seems like we need to update the xcode version for macOS CI to boot up. ref #223

Copy link
Member

@malept malept left a comment

Choose a reason for hiding this comment

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

I'm OK with the spirit of the PR but not the implementation.

  • asar is first and foremost a library, not a CLI. As such, it cannot process.exit in the middle.
  • this needs tests so that any future changes don't regress the change.

Reworked the error handling to make it more "proper" as well as
having added tests.
@Edythator
Copy link
Author

Hello, @malept.

Sorry for not having understood the project's layout. I've now updated the branch in order to more accurately fit the requirements that this project needs. Please let me know if you have any further concerns.

lib/filesystem.js Outdated Show resolved Hide resolved
bin/asar.js Outdated
const fileData = asar.extractFile(archive, filename)
if (typeof fileData === 'undefined') {
console.error(`The file "${filename}" was not found in the archive.`)
return
Copy link
Member

Choose a reason for hiding this comment

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

This is in the CLI part of the repository, so it makes more sense here to process.exit(1)... although if an exception is thrown, the changes here probably aren't needed.

Copy link
Author

@Edythator Edythator Jan 25, 2022

Choose a reason for hiding this comment

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

This is in the CLI part of the repository, so it makes more sense here to process.exit(1)... although if an exception is thrown, the changes here probably aren't needed.

Yeah, I couldn't really add a process.exit(1) to start with since that messes up the test.

Added an exception handler instead of relying on an if branch.
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.

4 participants