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

refactor: tackle corner cases and change variable names for better readability #38

Merged
merged 4 commits into from
Mar 25, 2024

Conversation

khareyash05
Copy link
Contributor

@khareyash05 khareyash05 commented Mar 12, 2024

Signed-off-by: Yash yash2010118@akgec.ac.in

  • Completed TODO for serveStatic
  • Added corner cases and error handling for deleting applications
  • Renamed variables so they are easier to read and understand their functionality
  • Added more error cases for applications

…adability

Signed-off-by: Yash <yash2010118@akgec.ac.in>
} => ({
success: 'Deploy Delete Succeed',
error: `Oops! It looks like the application (${app}) hasn't been deployed yet. Please deploy it before you delete it.`,
folderShouldntExist: `The folder shouldnt exist after deleting it.`,
Copy link
Contributor

Choose a reason for hiding this comment

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

remove these two newly added cases, as they are not required, as the response is for user not the developer.

Remove the logic for folderShouldntExist, appShouldntExist, as all the cases are handled properly.

Also Dont use multiple return statements.

Copy link
Contributor Author

@khareyash05 khareyash05 Mar 12, 2024

Choose a reason for hiding this comment

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

Hey ! Thanks for the review I really dont think the cases are being handled properly. for example when scaling it for future there may be cases when the system disconects or any other simultaneous processes may block the current proccesses. I think these cases will be need and thus multiple return statements are necessary. Let me know if there are any alternate thoughts you have to the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see we shouldnt send it to the user but we need to handle these cases at the backend side IMHO

src/api.ts Show resolved Hide resolved
@khareyash05 khareyash05 requested a review from Creatoon March 12, 2024 16:13
@viferga
Copy link
Member

viferga commented Mar 15, 2024

@khareyash05 for me it seems correct, I do not understand what @Creatoon said respect to the error messages. Can you update the conflicts to make it work again with the latest changes?

Once you do that I will merge your PR.

@khareyash05
Copy link
Contributor Author

@khareyash05 for me it seems correct, I do not understand what @Creatoon said respect to the error messages. Can you update the conflicts to make it work again with the latest changes?

Once you do that I will merge your PR.

Resolved the conflicts. Maybe we are unable to comprehend what @Creatoon wants to say. If there is something I might be missing. Let me know. Thanks @viferga

@viferga
Copy link
Member

viferga commented Mar 18, 2024

I passed the CI and after my change of considering all warnings as errors (of the linter), your CI execution failed:
https://github.com/metacall/faas/actions/runs/8294111642/job/22775896259?pr=38

Correct this linter issue and I will merge it.

@khareyash05
Copy link
Contributor Author

I passed the CI and after my change of considering all warnings as errors (of the linter), your CI execution failed: https://github.com/metacall/faas/actions/runs/8294111642/job/22775896259?pr=38

Correct this linter issue and I will merge it.

fixed. Kindly rerun the CI

@giarve
Copy link
Member

giarve commented Mar 24, 2024

I passed the CI and after my change of considering all warnings as errors (of the linter), your CI execution failed: https://github.com/metacall/faas/actions/runs/8294111642/job/22775896259?pr=38
Correct this linter issue and I will merge it.

fixed. Kindly rerun the CI

Done, sorry for the long wait

@viferga
Copy link
Member

viferga commented Mar 25, 2024

@khareyash05 seems to be still failing.

@viferga
Copy link
Member

viferga commented Mar 25, 2024

I am merging this and I will solve the issues by myself.

@viferga viferga merged commit 46c7802 into metacall:master Mar 25, 2024
1 check failed
@khareyash05
Copy link
Contributor Author

khareyash05 commented Mar 26, 2024

Thanks @viferga .

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