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

Repairing BASE64 encoded vCard version 3 #441

Merged
merged 2 commits into from
Dec 26, 2018
Merged

Conversation

Webbeh
Copy link
Contributor

@Webbeh Webbeh commented Dec 24, 2018

Follow up of
#415

Repairs vCards in V3 that have a BASE64 encoding.

@codecov
Copy link

codecov bot commented Dec 24, 2018

Codecov Report

Merging #441 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #441      +/-   ##
============================================
+ Coverage     98.87%   98.87%   +<.01%     
- Complexity     1809     1811       +2     
============================================
  Files            65       65              
  Lines          5160     5168       +8     
============================================
+ Hits           5102     5110       +8     
  Misses           58       58
Impacted Files Coverage Δ Complexity Δ
lib/Property.php 98.96% <100%> (+0.04%) 70 <0> (+2) ⬆️

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 5b86533...24bb625. Read the comment docs.

Copy link
Member

@evert evert left a comment

Choose a reason for hiding this comment

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

Looks great. Thank you very much. I added 2 tiny tweaks that you can accept right from the review (if you want them).

lib/Property.php Outdated Show resolved Hide resolved
lib/Property.php Outdated Show resolved Hide resolved
@Webbeh
Copy link
Contributor Author

Webbeh commented Dec 25, 2018

What's the point of the "offsetSet" function if you're not using it ? Just wondering.

Also the identity operator shouldn't be necessary in this case, as strtoupper() always returns a string, but I guess it doesn't hurt.

Applying the changes. Do you think we can see this merged in anytime soon ?

Co-Authored-By: Webbeh <info@geeq.ch>
@evert
Copy link
Member

evert commented Dec 26, 2018

What's the point of the "offsetSet" function if you're not using it ? Just wondering.

The offsetSet function is part of the ArrayAccess PHP interface. This interface's sole role is to make sure that $obj['item'] can be used.

Also the identity operator shouldn't be necessary in this case, as strtoupper() always returns a string, but I guess it doesn't hurt.

Apparently it's slightly faster, but it's probably negligible. As a general rule I find that using string comparisons (=== and !==) should be used always, except if you deliberately want to opt-out of strict checking and must use == for specific situations. Applying this rule strictly can avoid subtle bugs.

In this case neither of my suggestions actually did anything, it's just small stuff to help guard consistency. I also never used the 'suggest' feature before so I hoped you could just accept the suggestion with the click of a button ;)

@evert evert merged commit e064da1 into sabre-io:master Dec 26, 2018
@Webbeh
Copy link
Contributor Author

Webbeh commented Dec 26, 2018

It was not just "one click" but close enough :P

Did you have to override the offsetSet method ? I don't see what more it could do compared to standard arrays.

Yeah I know, it was just for consistency. It makes sense than the identity operator is slightly faster, since it doesn't have to bother with type casting. However, I'm assuming that if you provide two identical objects (let's say two strings with the value of "foo"), it still has to check whether the type is the same in both cases : in this one to check whether it's actually the same type or not, and with the equality operator to check whether to cast the object or not.

Thanks for the merge !

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.

2 participants