Skip to content

Conversation

@alex-friedl
Copy link
Collaborator

No description provided.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 98.919% when pulling 00c9104 on alex-friedl:phonegapTests into ebc9e89 on appfeel:master.

@appfeel
Copy link
Owner

appfeel commented Feb 9, 2017

Thanks, there is still missing tests when custom is string and when custom is something else (a number i.e.). In fact now that I read again the code, I see something not really nice there:

    let custom = typeof data.custom === 'string' ? { message: data.custom } : {};
    if (typeof data.custom === 'string') {
        custom = {
            message: data.custom,
        };
    } else if (typeof data.custom === 'object') {
        custom = Object.assign({}, data.custom);
    } else {
        custom = {
            data: data.custom,
        };
    }

What do you think if we change it to:

    let custom;
    if (typeof data.custom === 'string') {
        custom = {
            message: data.custom,
        };
    } else if (typeof data.custom === 'object') {
        custom = Object.assign({}, data.custom);
    } else {
        custom = {
            data: data.custom,
        };
    }

Would this affect to your branch? I propose that you implement this change and check if it's going to break your branch. Otherwise will implement in master but maybe some conflicts with this branch could arise.

@alex-friedl
Copy link
Collaborator Author

Looks good to me and does not break any tests. I added your suggestion to this branch.

The missing tests you describe were missing all along then, weren't they? Could you just add them to the master on your own? I would really appreciate that :)

@appfeel
Copy link
Owner

appfeel commented Feb 9, 2017

Hmm... that's quite strange, the coverage before your branch was 100%... Are you sure that they were missing?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 98.919% when pulling 777182c on alex-friedl:phonegapTests into ebc9e89 on appfeel:master.

@alex-friedl
Copy link
Collaborator Author

I think so?

Coverage on commit 873238 (Release 1.0.14)

Statements : 98.97% ( 193/195 )
Branches : 94.63% ( 141/149 )
Functions : 100% ( 42/42 )
Lines : 98.9% ( 180/182 )

Coverage on commit 777182

Statements : 98.99% ( 196/198 )
Branches : 94.84% ( 147/155 )
Functions : 100% ( 42/42 )
Lines : 98.92% ( 183/185 )

@appfeel appfeel merged commit eb35731 into appfeel:master Feb 12, 2017
@alex-friedl
Copy link
Collaborator Author

Thanks for merging! :)

Are you going to release 1.0.15 soon?

@appfeel
Copy link
Owner

appfeel commented Feb 13, 2017

Sure, I'm planning to implement the coverage remaining parts and publish (hope today, sure this week)

@alex-friedl
Copy link
Collaborator Author

Hi @appfeel

any progress on your part? Would you consider releasing a new version without the missing coverage if you can't find the time to implement it?

Regards
Alex

@appfeel
Copy link
Owner

appfeel commented Feb 24, 2017

Thanks @alex-friedl, released :)

@alex-friedl
Copy link
Collaborator Author

Thank you! :)

@appfeel
Copy link
Owner

appfeel commented Feb 24, 2017

I'd be happy to add you in contributors section in package if you have time to pull request it as you wish :)

@alex-friedl
Copy link
Collaborator Author

I'd be happy to be added :)

What exactly do you need me to do? Not quite sure I understand :)

@appfeel
Copy link
Owner

appfeel commented Feb 24, 2017

:) a new pull request with a package.json modified: https://docs.npmjs.com/files/package.json#people-fields-author-contributors

"contributors": [
  {
    "name": "optional",
    "email": "optional",
    "url": "optional"
  }
]

@alex-friedl
Copy link
Collaborator Author

Done! :)

#35

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