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

miqUncompressedId - fix [sprintf] expecting number but found string #10637

Merged
merged 3 commits into from Sep 9, 2016
Merged

miqUncompressedId - fix [sprintf] expecting number but found string #10637

merged 3 commits into from Sep 9, 2016

Conversation

himdel
Copy link
Contributor

@himdel himdel commented Aug 19, 2016

@chrisarcand reported seeing a [sprintf] expecting number but found string error..

sprintf doesn't typecast, and splitting a string yields two strings.. fixing to expect a string.

(alternatively we could convert it to a number but might get weird off-by-ones on 32 bit systems that way..)


Actually, the typecasting bit is a bit more complicated, it can treat a string as a number, as long as the native isNaN returns false for it..

@himdel
Copy link
Contributor Author

himdel commented Aug 19, 2016

@chrisarcand hope this fixes the problem...

Should it not, could you please do.. (in the browser console)

var bak = window.sprintf;
window.sprintf = function() { console.log(arguments); return bak.apply(window, arguments); };

and post the last one before it blows up?

@chrisarcand
Copy link
Member

It looks like this fixed one (or a few) of the error messages for sure!

I still get one more, clicking around:

Looks like your tests might be failing due to an unrelated issue, might want to give it a rebase and a kick.

@himdel
Copy link
Contributor Author

himdel commented Aug 28, 2016

Great, thanks! :) Rebased..

@chrisarcand
Copy link
Member

@himdel
Copy link
Contributor Author

himdel commented Aug 29, 2016

@chrisarcand that's not really related to the other issue, so I'd prefer to fix that in a separate PR.. But yup, noted :)

@chrisarcand
Copy link
Member

ok! LGTM from me, then 👍

@miq-bot
Copy link
Member

miq-bot commented Sep 6, 2016

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

sprintf doesn't typecast, and splitting a string yields two strings

alternatively we could convert it to a number but might get weird off-by-ones on 32 bit systems that way..
@miq-bot
Copy link
Member

miq-bot commented Sep 7, 2016

Checked commits https://github.com/himdel/manageiq/compare/d70ef268da93c0c3bf8cb00125d725b0f99d56dc~...37297cf38c84110bfc65e2ee15bb0b6ca2421975 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
0 files checked, 0 offenses detected
Everything looks good. 👍

@dclarizio
Copy link

Based on @chrisarcand's review, merging this.

@dclarizio dclarizio merged commit 2a0cc83 into ManageIQ:master Sep 9, 2016
@dclarizio dclarizio added this to the Sprint 46 Ending Sep 12, 2016 milestone Sep 9, 2016
@himdel himdel deleted the fix-miq-uncompressed branch September 10, 2016 22:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants