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 method to reduce ABC Metric size #6529

Merged
merged 3 commits into from Nov 7, 2017

Conversation

Projects
None yet
7 participants
@ashmaroli
Member

ashmaroli commented Nov 7, 2017

Assignment Branch Condition Size for the method on repo branches (as reported by RuboCop) :

  • master : 29.7
  • PR-branch: 20.1 13.64

@ashmaroli ashmaroli added the internal label Nov 7, 2017

Show outdated Hide outdated lib/jekyll/commands/serve.rb Outdated
adapt @envygeeks suggestion with a minor change
 - use parallel assignment via Hash#values_at
 - use nil checks to solve a semantics issue when using negated expressions
   with &&
 - output better message when an exception is raised

---

Though he suggested the use of Pathutil lib for both sanitizing the paths
to the SSL-Certificate & SSL-Key files, and for reading them, I elected to
abstract existing logic to a new private method.
@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Nov 7, 2017

Most people (including me) forget that RuntimeError is the default error. so if you just raise with just the message it'll throw RuntimeError by default and remove that RedundantException message.

envygeeks commented on lib/jekyll/commands/serve.rb in d589f46 Nov 7, 2017

Most people (including me) forget that RuntimeError is the default error. so if you just raise with just the message it'll throw RuntimeError by default and remove that RedundantException message.

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Nov 7, 2017

Owner

Perhaps., though, I only left it so because I thought Jekyll likes having everything explicitly stated

Owner

ashmaroli replied Nov 7, 2017

Perhaps., though, I only left it so because I thought Jekyll likes having everything explicitly stated

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Nov 7, 2017

TBQF if Jekyll wanted that error to be explicit it would be a Jekyll error rather than a RuntimeError, that error was originally put in by me as a place holder because Jekyll is the wild-west of source code, and I didn't really have time to implement a custom StandardError for it, or to check and see if the library that does runtime opt parsing did either.

envygeeks replied Nov 7, 2017

TBQF if Jekyll wanted that error to be explicit it would be a Jekyll error rather than a RuntimeError, that error was originally put in by me as a place holder because Jekyll is the wild-west of source code, and I didn't really have time to implement a custom StandardError for it, or to check and see if the library that does runtime opt parsing did either.

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Nov 7, 2017

Owner

Ah I see..
@parkr, if you're okay with not stating (the default) RuntimeError for a raise call explicitly, I'll gladly remove it.

Owner

ashmaroli replied Nov 7, 2017

Ah I see..
@parkr, if you're okay with not stating (the default) RuntimeError for a raise call explicitly, I'll gladly remove it.

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Nov 7, 2017

if you're okay with not stating (the default) RuntimeError for a raise call explicitly, I'll gladly remove it.

👍 Works for me! You can also rails an Errors::FatalException if you like. As long as Rubocop is happy with the "naked raise." (no explicit class), I'm cool with it.

parkr replied Nov 7, 2017

if you're okay with not stating (the default) RuntimeError for a raise call explicitly, I'll gladly remove it.

👍 Works for me! You can also rails an Errors::FatalException if you like. As long as Rubocop is happy with the "naked raise." (no explicit class), I'm cool with it.

@DirtyF

DirtyF approved these changes Nov 7, 2017

@DirtyF DirtyF added this to the v3.7.0 milestone Nov 7, 2017

@oe

oe approved these changes Nov 7, 2017

@DirtyF

This comment has been minimized.

Show comment
Hide comment
@DirtyF

DirtyF Nov 7, 2017

Member

@jekyllbot: merge +dev

Member

DirtyF commented Nov 7, 2017

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit 758ee7e into jekyll:master Nov 7, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ashmaroli ashmaroli deleted the ashmaroli:reduce-ABC-size branch Nov 8, 2017

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Nov 8, 2017

Member

Thanks for your inputs on this one @envygeeks

Member

ashmaroli commented Nov 8, 2017

Thanks for your inputs on this one @envygeeks

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