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

Add specs using pedicel pay #7

Merged
merged 117 commits into from
May 22, 2018
Merged

Conversation

kse-clearhaus
Copy link
Contributor

Added unit tests, small code cleanups.

Primary goal right now would be to add a few comments to the library, as well as clean up the code slightly.

Not to be merged yet.

lib/pedicel.rb Outdated

DEFAULTS = {
oids: {
intermediate_certificate: '1.2.840.113635.100.6.2.14',
leaf_certificate: '1.2.840.113635.100.6.29',
merchant_identifier_field: '1.2.840.113635.100.6.32',
merchant_identifier_field: '1.2.840.113635.100.6.32'
Copy link
Member

Choose a reason for hiding this comment

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

To keep diffs small, when adding another line, I prefer the trailing , 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So do i :)
Was just following rubocop recommendations. I'll change the rubocop warning, then.

lib/pedicel.rb Outdated
at+qIxUCMG1mihDK1A3UT82NQz60imOlM27jbdoXt2QfyFMm+YhidDkLF1vLUagM
6BgD56KyKA==
-----END CERTIFICATE-----
PEM
Copy link
Member

Choose a reason for hiding this comment

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

I like the idea to rename DEFAULTS[:apple_root_ca_g3_cert_pem] to DEFAULTS[:trusted_ca_pem] but I'd like to have APPLE_ROOT_CA_G3_CERT_PEM inside the Pedicel module.

raise ArgumentError 'invalid argument combination' unless \
!symmetric_key.nil? ^ ((!merchant_id.nil? ^ !certificate.nil?) && !private_key.nil?)
ca_certificate_pem: @config[:trusted_ca_pem], now: Time.now)
raise Pedicel::ArgumentError, 'invalid argument combination' unless \
Copy link
Member

Choose a reason for hiding this comment

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

I think it's perfectly fine to raise an ArgumentError; one wouldn't want to catch such an error (I think) when rescuing Pedicel::Errors?

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 agree! I'd missed the builtin ArgumentError :/

ca_certificate_pem: @config[:trusted_ca_pem], now: Time.now)
raise Pedicel::ArgumentError, 'invalid argument combination' unless \
!symmetric_key.nil? ^
((!merchant_id.nil? ^ !certificate.nil?) && private_key)
Copy link
Member

Choose a reason for hiding this comment

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

This breaks the alignment of the comments below.

Do not fear a few long lines 🙂

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 can live with the long lines then :)
Though this break was more for clarity, with which it didn't help, of course. Going to look at it again.

Copy link
Member

Choose a reason for hiding this comment

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

At first sight, I did only notice the wrapping. Now I notice !private_key.nil? -> private_key.

I don't know if I like it. Generally I certainly do because if foo reads better than if !foo.nil?, but since !x.nil? is necessary earlier on the line (because ^ will not coerce), I don't really know.

I'm indifferent and I guess you prefer the coercing version, let's go with it 🙂

@@ -6,10 +6,15 @@ def ephemeral_public_key
Base64.decode64(@token['header']['ephemeralPublicKey'])
end

# Decrypt self.encrypted_data() finding the key from either
# symmetric_key, og
#
Copy link
Member

Choose a reason for hiding this comment

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

Rather no comment IMO.

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 agree that the comment I wrote does not make sense :)
But do you mean no comments at all?
I was considering writing some simple rdoc documentation.

Copy link
Member

Choose a reason for hiding this comment

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

I'd like to see comments exactly when I need to read the comments in order to understand the code.

My reasoning:

  • If a method needs a comment to understand what the method is doing, then rename the method name and/or parameters.
  • If I need to really understand, I'd read the source code anyway.
  • Comments will over time drift from the actual code since nothing enforces (or tests) that the comments are aligned with the code. Thus, over time, comments will inevitably be wrong. And, rather no comments than wrong/misleading/... comments.

symmetric_key = symmetric_key(
private_key: private_key,
merchant_id: merchant_id
)
Copy link
Member

Choose a reason for hiding this comment

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

Why? The initial check ("assert style-ish" programming) shouldn't be carried "all over" IMO.

symmetric_key can itself derive the merchant_id; no reason to copy the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a bit of pedantry I guess.
When called with (symmetric_key and private_key), it would fail in symmetric_key(), since neither certificate nor merchant_id was defined, so I added

+      if private_key && merchant_id
+        symmetric_key = symmetric_key(
+          private_key: private_key,
+          merchant_id: merchant_id
+        )

I guess the check above came as a side-effect of that.

Though, it would probably make sense to argue that the function should fail if passed only symmetric_key and private_key. which it would with the original code.

My thought when I wrote it was probably that it wasn't entirely clear.

Copy link
Member

Choose a reason for hiding this comment

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

The initial check would fail if symmetric_key && private_key.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it doesn't. A unit test like the following fails:

describe 'decrypt'
  context 'should fail on' do
    it 'excessive arguments' do
      backend, merchant, token, _data = PedicelPay::Helper.generate_all

      p = Pedicel::EC.new(
        JSON.parse(token.to_json),
        config: Pedicel::DEFAULTS.merge(
          trusted_ca_pem: backend.ca_certificate
        )
      )

      merchant_id = merchant.merchant_id
      certificate = merchant.certificate
      private_key = merchant.key
      symmetric_key = p.symmetric_key(
        certificate: merchant.certificate,
        private_key: merchant.key
      )
      expect do
        p.decrypt(
          symmetric_key: symmetric_key,
          private_key: private_key
        )
      end.to raise_error(ArgumentError)
    end
  end
end

With symmetric_key and private_key the right side evaluates to false, while the left side evaluates to true, satisfying the XOR.

Copy link
Member

Choose a reason for hiding this comment

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

Indeed. The logic is broken.

raise EcKeyError, "invalid format of ephemeralPublicKey (from token) for EC: #{e.message}"
rescue StandardError => e
raise EcKeyError,
"invalid format of ephemeralPublicKey (from token) for EC: #{e.message}"
Copy link
Member

Choose a reason for hiding this comment

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

Is rubocop asking for this formatting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, both regarding line-lengths and StandardError.
I would probably prefer omitting StandardError, so I'd be happy to change it back :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would changing it back be okay?

"private_key curve '#{privkey.group.curve_name}' differ from " \
"ephemeralPublicKey (from token) curve '#{pubkey.group.curve_name}'"
"private_key curve '#{privkey.group.curve_name}' differ from " \
"ephemeralPublicKey (from token) curve '#{pubkey.group.curve_name}'"
Copy link
Member

Choose a reason for hiding this comment

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

Is rubocop asking for this formatting?

I think I might even prefer it glued into one long line. Easier for grep'ing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, sort of. :)

raise CertificateError, "invalid PEM format of certificate: #{e.message}"
rescue StandardError => e
raise CertificateError,
"invalid PEM format of certificate: #{e.message}"
Copy link
Member

Choose a reason for hiding this comment

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

Is rubocop asking for this formatting?

lib/pedicel.rb Outdated
module Pedicel
class Error < StandardError; end
class TokenFormatError < Error; end
class SignatureError < Error; end
class VersionError < Error; end
class CertificateError < Error; end
class EcKeyError < Error; end
class ArgumentError < Error; end
Copy link
Member

Choose a reason for hiding this comment

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

I think it shouldn't be needed; see below.


raise ReplayAttackError, "token signature time indicates a replay attack (age #{now-cms_signing_time})" unless signing_time_ok?(now: now)
raise VersionError, "unsupported version: #{version}" unless
SUPPORTED_VERSIONS.include?(@token['version'])
Copy link
Contributor

Choose a reason for hiding this comment

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

I would combine these two lines, rather than having a line break after unless, or replace them with an unless ...\n ...\n end structure.

🚲 🏠

Copy link
Contributor

Choose a reason for hiding this comment

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

It is in the category "do not fear a few long lines", mentioned by @ct-clearhaus earlier.

def private_key_class
{EC_v1: OpenSSL::PKey::EC, RSA_v1: OpenSSL::PKey::RSA}[version]
{ EC_v1: OpenSSL::PKey::EC, RSA_v1: OpenSSL::PKey::RSA }[version]
Copy link
Contributor

Choose a reason for hiding this comment

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

The keys :EC_v1 and :RSA_v1 are symbols, but version appears to return a string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Version currently returns a string:

    def version
      @token['version']&.to_sym
    end

This is a problem, however, since it is used in a string above:

      raise VersionError, "unsupported version: #{version}" unless SUPPORTED_VERSIONS.include?(@token['version'])

I'll change to consistent symbol use unless the other is preferred :)

Copy link
Member

Choose a reason for hiding this comment

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

Why is that a problem?

irb(main):001:0> "#{:foobar}"
=> "foobar"

Copy link
Member

Choose a reason for hiding this comment

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

Also version returns a symbol 🙂

Copy link
Contributor

@mt-clearhaus mt-clearhaus Apr 20, 2018

Choose a reason for hiding this comment

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

Hmm, when I reviewed and searched for "def version" on the page https://github.com/clearhaus/pedicel/pull/7/files , I got the following:

screenshot_2018-04-20_13-58-47

i.e. [@token['applicationData']].pack('H*'). I still do. Weird. I have checked out the branch locally and see @token['version']&.to_sym.

My point was:

> { EC_v1: 1, RSA_v1: 2 }['EC_v1']
=> nil

Copy link
Contributor

Choose a reason for hiding this comment

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

Never mind 🙂 Notice on the screenshot that def version is on line 24, whereas [@token['applicationData']].pack('H*') is on line 47. GitHub collapsed the lines in between. Thanks to @ct-clearhaus for noticing.

Copy link
Member

Choose a reason for hiding this comment

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

I was apparently cheated too. First after thinking why applicationData had to do with version my eyes could see 🙂

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 definitely meant symbol, but wrote string :)

It turns out to not be a problem, but I got sidetracked and looked at different issues here.
I had simply assumed that:

puts("Hello " + :world)

would be functionally similar to:

puts("Hello #{:world}")

@@ -89,22 +83,22 @@ def decrypt_aes(key:)
else
require 'aes256gcm_decrypt'

Aes256GcmDecrypt::decrypt(encrypted_data, key)
Aes256GcmDecrypt.decrypt(encrypted_data, key)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, both . and :: work for invoking a singleton method in a module. I wonder which one is the most blessed? 🙂

Copy link
Member

Choose a reason for hiding this comment

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

true if validate_signature(now: now)
rescue
true if verify_signature(now: now)
rescue StandardError => _
Copy link
Contributor

Choose a reason for hiding this comment

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

Is => _ not superfluous?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, will be fixed along with standarderror

@@ -171,18 +170,19 @@ def self.verify_x509_chain(root:, intermediate:, leaf:)
raise SignatureError, 'invalid chain of trust' unless valid_chain
end

def self.verify_signed_time(signature:, now:)
def self.verify_signed_time(signature:, now:, config: Pedicel.config)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe also s/Pedicel.config/config/ here, like it has been changed in other places?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The plan was to have config: Pedicel.config as default for functions that require it, which means functions use config to refer to the parameter, which by default is Pedicel.config.

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 would prefer letting it be Pedicel.config, simply because it's easier to see where it comes from. What do you think?


predicate(:hex?) { |value| !Regexp.new(/\A[a-f0-9-]*\z/i).match(value).nil? }
predicate(:hex?) do |value|
if Regexp.new(/\A[a-f0-9-]*\z/i).match(value).nil?
Copy link
Contributor

Choose a reason for hiding this comment

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

Dash is not hex.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, trailing dash was an error.


predicate(:yymmdd?) do |value|
return false unless value.length == 6
Time.new(value[0..1],value[2..3], value[4..5]).is_a?(Time) rescue false
unless value.length == 6 # rubocop:disable Style/IfUnlessModifier
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want these rubocop magic knobs? We have not used Rubocop, at least in PCI, the last two years. Are we starting to?

Copy link
Member

Choose a reason for hiding this comment

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

We could use codeclimate ...

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 like having some style-checker, but it's not up to me.

Is there a specific reason to use CodeClimate instead on rubocop?
Rubocop seems to work well for me. It's the ruby language that generates the errors.

Copy link
Contributor

@mt-clearhaus mt-clearhaus left a comment

Choose a reason for hiding this comment

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

A lot of fine changes and specs, but also a lot of controversial Rubocop-inspired weirdness. I will let @ct-clearhaus take steering on style, as he has written most of our code (in general) and started this repo.


cms_signing_time.between?(now - delta, now + delta)
# Deliberately ignoring leap seconds.
end
Copy link
Member

Choose a reason for hiding this comment

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

Why should signing_time_ok? disappear?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. I guess the predicate signing_time_ok? would be a good choice to keep, but as it is, the functionality existed in self.verify_signed_time.

Copy link
Member

Choose a reason for hiding this comment

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

I see. And agree.

I think I tried making the replay-check part of the validation but gave up. (Notice cms_signing_time is left without an implementation 🙂)

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 agree that it could be argued that the replay-check could be part of the validation :)
I viewed the validation as a syntax check, as opposed to a semantic check.

unless intermediate
raise SignatureError, "no intermediate certificate found (OID #{Pedicel.config[:oids][:leaf_certificate]})"
raise SignatureError, "no intermediate certificate found (OID #{config[:oids][:leaf_certificate]})"
Copy link
Member

Choose a reason for hiding this comment

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

I subjectively like the class/module name to specify the scope mostly because it is much easier to understand where to find the method IMO; thus, concerning "readability".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, ruby is funny like that.
Hmm. There's also the question that a class function fetches an instance variable:

  def self.config
    @config ||= DEFAULTS
  end

I'm just going to think on this for a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case config is an argument to the function.

Copy link
Contributor Author

@kse-clearhaus kse-clearhaus left a comment

Choose a reason for hiding this comment

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

It's difficult to get an overview of the coverage of the tests, but I believe the tests are quite comprehensive.

raise SignatureError, 'root certificate has not issued intermediate certificate'
end
def self.verify_root_certificate(root:, trusted_root:)
raise SignatureError, 'root certificate is not trusted' unless root == trusted_root
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 don't recall, is this a deep check that one certificate matches the other correctly (does the certificate have the same issuer, public key, subject name etc.) or just by object matching?

I assume it's the first. :)

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 be surprised if it's not ensuring that the certs are "as certs" the same, but I'll dig deeper to ensure.

Copy link
Member

Choose a reason for hiding this comment

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

Fixed by 76c7555

.extensions
.find { |x| x.oid == config[:oids][:merchant_identifier_field] }
&.value # Hex encoded Merchant ID plus perhaps extra non-hex chars.
&.delete('^[0-9a-fA-F]') # Remove non-hex chars.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this just in case the certificate is bad or could there actually be non-hex characters there?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for asking! 💯

In the spreedly/Gala example there are non-hex chars. They fixed it by skipping these initial non-hex chars: https://github.com/spreedly/gala/blob/master/lib/gala/payment_token.rb#L87

This was my attempt at a more general solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How peculiar! 👍

backend
end

after (:all) { Pedicel.config.merge!(trusted_ca_pem: Pedicel::APPLE_ROOT_CA_G3_CERT_PEM) }
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 have to admit, I'm not a huge fan of this.
My preference would to not have any global variables (class level), so only instance variables are set.

Copy link
Member

Choose a reason for hiding this comment

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

#10 +41 -90 lines, so I'm pretty happy 🙂

value&. # Hex encoded Merchant ID plus perhaps extra non-hex chars.
delete("^[0-9a-fA-F]") # Remove non-hex chars.
cert
.extensions
Copy link
Member

Choose a reason for hiding this comment

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

There are good reasons for both:

I'm 🤷‍♂️ for which to choose (recently perhaps leaning towards the reasoning in rubocop/ruby-style-guide#176 (comment)). I expect (because git blame 😛) that @kse-clearhaus prefer the "dot in the front" style (which, depending on your way of thinking, could make more sense when used with & as the safe navigation operator). @kse-clearhaus can you confirm?

Copy link
Member

@ct-clearhaus ct-clearhaus May 18, 2018

Choose a reason for hiding this comment

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

I fell into a crater.

Think about

ary = [1,2,3]

or alike through this.

If something is commented out, then it's because it doesn't work.

Comments

#ary \ # Single-line comment.
#  .slice(5..-1) \
#  &.first \
#  &.zero?

ary \
  # Single-line comment.
  .slice(5..-1) \
  &.first \
  &.zero?

#ary \
#  # Multi-line
#  # comment.
#  .slice(5..-1) \
#  &.first \
#  &.zero?

ary # Single-line comment.
  .slice(5..-1)
  &.first
  &.zero?

#ary
#  # Single-line comment.
#  .slice(5..-1)
#  &.first
#  &.zero?

#ary
#  # Multi-line
#  # comment.
#  .slice(5..-1)
#  &.first
#  &.zero?

ary. # Single-line comment.
  slice(5..-1)&.
  first&.
  zero?

ary.
  # Single-line comment.
  slice(5..-1)&.
  first&.
  zero?

ary.
  # Multi-line
  # comment.
  slice(5..-1)&.
  first&.
  zero?

Clear winner: "trailing ."

Commenting out lines

ary \
  #.slice(5..-1) \
  &.first \
  &.zero?

#ary \
#  .slice(5..-1) \
#  &.first \
#  #&.zero?

#ary \
#  .slice(5..-1) \
#  &.first \
#  #&.zero?

ary
  #.slice(5..-1)
  &.first
  &.zero?

ary
  .slice(5..-1)
  &.first
  #&.zero?

ary
  #.slice(5..-1)
  #&.first
  #&.zero?

ary.
  #slice(5..-1)&.
  first&.
  zero?

#ary.
#  slice(5..-1)&.
#  first&.
#  #zero?

#ary.
#  #slice(5..-1)&.
#  #first&.
#  #zero?

As for "trailing \ leading ." and "trailing .": Notice the inconsistency:
iff you comment out the last line, things will break.

REPL pasting

ary \
  .slice(5..-1) \
  &.first \
  &.zero?
ary.
  slice(5..-1)&.
  first&.
  zero?
ary
  .slice(5..-1)
  &.first
  &.zero?

In this, 1 < m < n < 4 (and as for Array#[], "4 = -1").

Things you could do and why

The "whys" are just examples.

Pasting line 1

What's that ary thingy?

Pasting line 1..m:

It fails at some point, let me divide and conquer to see some intermediate
value.

Pasting line m..-1:

I have this toy ary at hand in my REPL, modify this the same way as would
happen with the more complex one that fails.

Pasting line m..n:

As above, but divide and conquer to understand what is happening.

Pasting line m..m:

What's this thing doing to my toy ary?

Support matrix

                        1       1..m    m..-1   m..n    m..m
                        -----   -----   -----   -----   -------
 trailing , leading .   + (a)   + (a)   +       + (a)   + (a)
           trailing .   + (b)   + (b)   + (c)   + (c)   + (b,c)
            leading .   +       -       -       -       +

(a): After the trailing \, hit enter (again).
(b): Delete the trailing ..
(c): Add a . in the REPL before pasting.

Copy link
Member

Choose a reason for hiding this comment

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

Just updated the above.

I think that it's more important to have comments in code (and that it's possible to comment the code "freely") than it is to be able to debug code easily (by either commenting out lines or C&P'ing to a REPL).
Reasons:

  • The comments are supposed to exist for a long time and be read multiple times (by a later self)
  • Debugging is (hopefully) done less frequently.
  • It's not too hard to also comment out a trailing \ or ..
  • It's not too hard to add a . or delete a trailing \ or . in a REPL.

Thus, I think I still lean towards "trailing .". But!

Assume you care more about commenting out code than pasting in a REPL. Then:

Why not get the benefits from "leading ." if you don't need the "comment freedom"?

"Because consistency" and "your eyes should only care about one" are both rather awful reasons IMO. Seasoned developers can for sure easily read both.

My thinking: Stop commenting out code! Drop in binding.pry on the line before, and paste into the REPL. That's usually a far superior way to understand code.

Thus, "trailing ." is the winner for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the above, I would agree that trailing . is preferable.

However, visually and semantically, I still much prefer a leading ..
In a sense, I read a leading . as a self-sufficient line, you start somewhere and you know where you're going.
I read the . as something like and then do. I read

ary
  .slice(5..-1)
  &.first
  &.zero?

as

Take ary,
then slice it,
then first,
then zero?

I read

ary. # Single-line comment.
  slice(5..-1)&.
  first&.
  zero?

as

Take ary then,
slice it then,
first then,
zero?

Where I clearly prefer the former.

With regards to:

Leading dots are an antipattern:

  1. They require the continuation to be present at precisely next line. (I.e. you cannot put more than one in-line comment in between.)
  2. They break mental context by requiring one to always analyze one more line of code.
  3. Most importantly, code written with leading dots cannot be copied and pasted to irb/pry.
  1. That's true. But I don't see that as a major obstacle. If you're chaining complex enough functions
    that you feel like you need to add multiline comments between them, you could probably refactor.
  2. They don't break mental context, the indentation on the next line indicate if it continues or not, at the very worst you can look at the first character.
  3. True, but when I'm in ViM I can just select the lines and shift + j to join them.

I think the most important part is that we're consistent about our choice. So I'd gladly use trailing .s :)

Copy link
Member

Choose a reason for hiding this comment

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

You cannot Join lines if there are comments 🙂

ct-clearhaus and others added 2 commits May 22, 2018 14:06
This is to accommodate for the concerns raised by @kse-clearhaus in
#7 (comment)

See also ruby/openssl#158
Copy link
Member

@ct-clearhaus ct-clearhaus left a comment

Choose a reason for hiding this comment

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

I'm happy. What about you @kse-clearhaus and @mt-clearhaus?

@mt-clearhaus mt-clearhaus self-requested a review May 22, 2018 12:53
Copy link
Contributor

@mt-clearhaus mt-clearhaus left a comment

Choose a reason for hiding this comment

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

👍

@ct-clearhaus ct-clearhaus merged commit b1fcc2a into master May 22, 2018
@ct-clearhaus ct-clearhaus deleted the add-specs-using-pedicel-pay branch May 22, 2018 13:29
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.

3 participants