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

Remove one try-catch #3052

Merged
merged 3 commits into from Aug 13, 2019
Merged

Conversation

KSXGitHub
Copy link
Contributor

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:

Description

I changed the code from this:

function rollup(...): Promise<...> {
  try {
    ...
  } catch (err) {
    return Promise.reject(err);
  }
}

to this:

async function rollup(...): Promise<...> {
  ...
}

@codecov
Copy link

codecov bot commented Aug 12, 2019

Codecov Report

Merging #3052 into master will decrease coverage by <.01%.
The diff coverage is 92.2%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3052      +/-   ##
==========================================
- Coverage    88.7%   88.69%   -0.01%     
==========================================
  Files         165      165              
  Lines        5718     5716       -2     
  Branches     1744     1744              
==========================================
- Hits         5072     5070       -2     
  Misses        388      388              
  Partials      258      258
Impacted Files Coverage Δ
src/rollup/index.ts 93.65% <92.2%> (-0.07%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2dfcb3b...2257a0a. Read the comment docs.

@shellscape
Copy link
Contributor

Thanks for the PR 🍺

It's super important to explain the motivation for your change. Please add a description to the pull request to explain why you made the change, what benefits it will have moving forward, and why it's better than the code that previously existed.

@KSXGitHub
Copy link
Contributor Author

@shellscape I don't quite understand your question. It's a simple refactoring, what's more do I have to explain?

@shellscape
Copy link
Contributor

(I'm sure you know this, but for the sake of being verbose) Refactoring is typically for any number of reasons:

  1. the code is inefficient
  2. subjective preference of one code structure/paradigm over another or existing
  3. preparation for larger/other changes to the codebase
  4. elimination of unneeded code

When you present a refactor, you should always explain and justify why the refactor is necessary. That's helpful to maintainers that are evaluating your change, and any onlookers that may be tracking the project's progress. It's also generally considered good form to thoroughly explain a pull request, and why we have a pull request template that encourages people to do so.

Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

I think it is a good change, considering we want to move to async-await anyway, and saves us three lines of code 😜

@shellscape
Copy link
Contributor

@lukastaegert for whatever reason, introducing this change is causing 4 of the tests to fail.

@lukastaegert
Copy link
Member

I fear we have some blinking tests here, which is unrelated to this PR. I guess I have to look into this first before we merge any PRs. I think there is a race condition that can lead to undeterministic chunking in one test.

@lukastaegert lukastaegert merged commit 04b7d52 into rollup:master Aug 13, 2019
@KSXGitHub KSXGitHub deleted the refactor-remote-try-catch branch August 13, 2019 06:46
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.

None yet

3 participants