Skip to content

Cast AccountNumber string to integer #158

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.

Sign up for GitHub

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

Open
samuel02 wants to merge 3 commits into
base: development
Choose a base branch
from
Open

Cast AccountNumber string to integer #158

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d6c099b
Cast AccountNumber string to integer
samuel02 Jun 4, 2018
26042da
Improve tests for AccountNumber
samuel02 Jun 15, 2018
51bbce1
Raise in case of empty string
samuel02 Jun 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions lib/fortnox/api/types.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,15 @@ module Types
AccountNumber = Strict::Int
.constrained(gteq: 0, lteq: 9999)
.optional
.constructor do |input|
Copy link
Member

@ehannes ehannes Jun 18, 2018

Choose a reason for hiding this comment

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

This constructor is now 9 lines. Can we create a separate method for this? I'm pretty sure Rubocop will complain about this :)

Copy link
Member

@d-Pixie d-Pixie Jun 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Look ma, one-liner ...

.constructor do |input|
  input.to_s.empty? ? nil : Integer(input.to_s)
end

Optimizing the logic for brevity and skipping the rescue. No one should send random user input directly at this and if they do that is their problem. We are not end user facing, we are developer facing and can expect that the input has already been sanitised. So allow for reasonable things - like nil and string/symbol versions - where it makes sense, but do not try to defend from all bad inputs.

unless input.nil? || input.to_s.empty?
Copy link
Member

@ehannes ehannes Jun 18, 2018

Choose a reason for hiding this comment

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

Are these cases tested?

Integer(input)
else
input
end
rescue ArgumentError
input
Copy link
Member

@ehannes ehannes Jun 18, 2018

Choose a reason for hiding this comment

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

Is this case tested?

Copy link
Member

@d-Pixie d-Pixie Jun 18, 2018

Choose a reason for hiding this comment

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

Not all valid/invalid cases are tested. We could test for everything but right now, with my suggested changes, we do test the two main cases: Things that respond to to_s and has a valid value will convert, the rest will cause a Dry types constraint error...

end

ArticleType = Strict::String
.constrained(included_in: ArticleTypes.values)
Expand Down
12 changes: 12 additions & 0 deletions spec/fortnox/api/types/account_number_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,16 @@
context 'when AccountNumber created with a negative number' do
include_examples 'raises ConstraintError', -1
Copy link
Member

@ehannes ehannes Jun 14, 2018

Choose a reason for hiding this comment

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

Same here. We should probably keep these as integers. We only need to test the casting once.

end

context 'when AccountNumber created with an invalid string' do
Copy link
Member

@d-Pixie d-Pixie Jun 18, 2018

Choose a reason for hiding this comment

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

Firstly this should be describe Fortnox::API::Types::AccountNumber surely? The spec is named as specifically for that anyway :) That will also mean that we can skip the "when AccountNumber ..." for all contexts and you will have just context 'created with an invalid string' do for example.

Basically this test could be anything that we don't explicitly allow, so make it with invalid input and just make sure we get a Dry types error for now.

Copy link
Member

@ehannes ehannes Jun 18, 2018

Choose a reason for hiding this comment

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

Yes, it should be describe Fortnox::API::Types::AccountNumber. My bad!

include_examples 'raises ConstraintError', 'foo'
end

context 'when AccountNumber created with valid string' do
subject { klass['1234'] }

it 'casts it to a number' do
Copy link
Member

@d-Pixie d-Pixie Jun 18, 2018

Choose a reason for hiding this comment

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

We don't really care HOW it does something here, just that we get the correct result. So this could be:

context 'created with valid string' do
  subject { klass['1234'] }
  it { is_expected.to eq 1234 }
end

Utilizing the readability of RSpecs one line syntax.

Copy link
Member

@d-Pixie d-Pixie Jun 18, 2018

Choose a reason for hiding this comment

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

Actually if we consider how we do things we can change it to 'created with valid value that responds to to_s', since that is actually what we allow.

is_expected.to eq 1234
end
end
end