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

perf: handle the case if the file is not present. #812

Merged
merged 8 commits into from
Oct 9, 2023

Conversation

ayushnau
Copy link
Contributor

@ayushnau ayushnau commented Sep 19, 2023

  1. if the file will not be present than the file will be created .
  2. if the file is catched due to the error of cant be read then that error will be printed.

I have worked on the issue no. #800 .

image

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.

@ayushnau ayushnau changed the title handle the case if the file is not present. perf : handle the case if the file is not present. Sep 19, 2023
@ayushnau ayushnau changed the title perf : handle the case if the file is not present. perf: handle the case if the file is not present. Sep 19, 2023
Copy link
Member

@Souvikns Souvikns left a comment

Choose a reason for hiding this comment

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

@Ayush2020012016 tests are failing due to lint issues. To fix lint issues just run
npm run lint:fix

@peter-rr
Copy link
Member

peter-rr commented Sep 21, 2023

@Ayush2020012016 @Souvikns
Hey guys, I'm not really sure about the approach of this PR 🤔 I mean, some months ago I came up with the same idea behind the issue #800 and then realised that those catch blocks were left empty on purpose: we don't want the program to stop or show an error when actually there is not an error happening (a new file is going to be created), we just want the code to keep executing. That's why those comments were added (same logic is applied here at the same file).

If we follow that logic then we should leave the creation of the file outside the catch, just to not be treated as an error (when readFile fails because the file does not exist, it means we need to create the file and then keep the execution going). And just including the permissions error inside the catch, since that's actually an error.

WDYT? Any thoughts on this?

Copy link
Member

@peter-rr peter-rr left a comment

Choose a reason for hiding this comment

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

Here my suggestion :)

if (e.code === 'EACCES') {
console.error('Permission denied to read the file. You do not have the necessary permissions.');
} else {
await writeFile(fileNameToWriteToDisk, asyncApiFile, { encoding: 'utf8' });
Copy link
Member

Choose a reason for hiding this comment

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

I would leave the "creation of the file" outside the catch, so it's not be treated as an error like I explain here.

Choose a reason for hiding this comment

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

ok @peter-rr will do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@peter-rr should i create file even though there is the permission error? means even though we dont have permission to read the file we just created it. it will look like this.
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

till then i have ran the lint command please consider merging it.

Copy link
Member

Choose a reason for hiding this comment

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

@peter-rr should i create file even though there is the permission error? means even though we dont have permission to read the file we just created it. it will look like this.

Nope, we should not create the file if a permission error occurs. In that case we should throw the error so that we exit the execution and the file is not created then:

} catch (e) {
      if (e.code === 'EACCES') {
        throw e;
      }
}
await writeFile(fileNameToWriteToDisk, asyncApiFile, { encoding: 'utf8' });
console.log(`Created file ${fileNameToWriteToDisk}...`);

Another alternative we have is using this.error instead, since OCLIF makes use of it as console.error + throw e:

} catch (e) {
      if (e.code === 'EACCES') {
        this.error('Permission denied to read the file. You do not have the necessary permissions.');
      }
}
await writeFile(fileNameToWriteToDisk, asyncApiFile, { encoding: 'utf8' });
console.log(`Created file ${fileNameToWriteToDisk}...`);

Copy link
Contributor Author

@ayushnau ayushnau Sep 28, 2023

Choose a reason for hiding this comment

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

ok will do that. @peter-rr

@ayushnau
Copy link
Contributor Author

ayushnau commented Sep 30, 2023

image
@peter-rr @Souvikns

@ayushnau
Copy link
Contributor Author

ayushnau commented Oct 2, 2023

image @peter-rr @Souvikns

@peter-rr can you tell me what I am doing wrong i really dont understand.

@peter-rr
Copy link
Member

peter-rr commented Oct 3, 2023

@peter-rr can you tell me what I am doing wrong i really dont understand.

I think you are doing it right and getting the expected behaviour: if the user does not have the necessary permissions to read the file, then the error is thrown, the flow execution stops and a new file is not created. Otherwise the already existing file could be overwritten by a new file with the same name. Am I answering your question? @ayushnau

@ayushnau
Copy link
Contributor Author

ayushnau commented Oct 3, 2023

@peter-rr can you tell me what I am doing wrong i really dont understand.

I think you are doing it right and getting the expected behaviour: if the user does not have the necessary permissions to read the file, then the error is thrown, the flow execution stops and a new file is not created. Otherwise the already existing file could be overwritten by a new file with the same name. Am I answering your question? @ayushnau

yes @peter-rr that is what i was seeking. @Souvikns please merge this.

Copy link
Member

@peter-rr peter-rr left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 9, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@Souvikns
Copy link
Member

Souvikns commented Oct 9, 2023

/rtm

@asyncapi-bot asyncapi-bot merged commit ba9df40 into asyncapi:master Oct 9, 2023
11 checks passed
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 0.58.8 🎉

The release is available on:

Your semantic-release bot 📦🚀

@Souvikns
Copy link
Member

@allcontributors please add @ayushnau for code

@allcontributors
Copy link
Contributor

@Souvikns

I've put up a pull request to add @ayushnau! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants