The Wayback Machine - http://web.archive.org/web/20201017053312/https://github.com/wulkano/Kap/pull/941
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

Improve error handling for export encoding #941

Open
wants to merge 4 commits into
base: master
from

Conversation

@andreidobrinski
Copy link

@andreidobrinski andreidobrinski commented Oct 1, 2020

Fixes #888

Hope this helps!

@sindresorhus sindresorhus changed the title check if error text exists before returning Improve error handling for export encoding Oct 2, 2020
@sindresorhus sindresorhus requested a review from karaggeorge Oct 2, 2020
@virtuoushub
Copy link

@virtuoushub virtuoushub commented Oct 2, 2020

@andreidobrinski To get this PR to pass the CI checks, we just need to remove the whitespace from the updated file. e.g.:

From 7dc60d46521f7be77c022dc15c23ebde18ab33b4 Mon Sep 17 00:00:00 2001
From: Omitted <bots@scrape.this>
Date: Fri, 2 Oct 2020 08:33:28 -0400
Subject: [PATCH] style(whitespace): fix whitespace errors

breaking CI
See: https://app.circleci.com/pipelines/github/wulkano/Kap/168/workflows/047525cc-f95b-41aa-bf6e-be40c377aa7c/jobs/1721/parallel-runs/0/steps/0-103
---
 main/utils/encoding.js | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/main/utils/encoding.js b/main/utils/encoding.js
index 3c48803..0307d57 100644
--- a/main/utils/encoding.js
+++ b/main/utils/encoding.js
@@ -15,11 +15,11 @@ const getEncoding = async filePath => {
     await execa(ffmpegPath, ['-i', filePath]);
   } catch (error) {
     const errorText = /.*: Video: (.*?) \(.*/.exec(error.stderr);
-    
+
     if (!errorText) {
       throw error;
     }
-    
+
     return errorText[1];
   }
 };
-- 
2.27.0
@andreidobrinski
Copy link
Author

@andreidobrinski andreidobrinski commented Oct 2, 2020

Thanks @virtuoushub and @sindresorhus for the feedback.

I updated the PR to remove whitespace.

Thanks also for your work, I love the app!

if (!errorText) {
throw error;
}
return errorText[1];

This comment has been minimized.

@virtuoushub

virtuoushub Oct 4, 2020

@andreidobrinski, almost there. Looking at the CI's error output, I believe the linter wants:

    const errorText = /.*: Video: (.*?) \(.*/.exec(error.stderr);
    if (!errorText) {
      throw error;
    }

    return errorText[1];

We can use yarn test locally to verify.

This comment has been minimized.

@andreidobrinski

andreidobrinski Oct 4, 2020
Author

Thanks for the tip! I committed the update :)

@sindresorhus
Copy link
Member

@sindresorhus sindresorhus commented Oct 4, 2020

Still failing linting. Please always run npm test locally after doing changes.

@andreidobrinski
Copy link
Author

@andreidobrinski andreidobrinski commented Oct 4, 2020

Should be ok now. Thanks!

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

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.