search

stampit-org / stamp

Stamps - better OOP model
https://stampit.js.org
MIT License
25 stars 3 forks source link

Repo updates #51

Closed PopGoesTheWza closed 4 years ago

PopGoesTheWza commented 4 years ago

fixes

PopGoesTheWza commented 4 years ago

@koresar I do not think the failing npm run test is my doing. I managed to fix a bug in merge though.

PopGoesTheWza commented 4 years ago

@koresar Tada! All tests passed.

Please review my quick fixes and advise for a cleaner solution.

PopGoesTheWza commented 4 years ago

My understanding of what goes on in mergeOne is that when src is undefined, dst is lost.

koresar commented 4 years ago

You might have fixed a bug here btw. However, I'd keep such changes separate.

On Fri., 1 Nov. 2019, 20:56 PopGoesTheWza, notifications@github.com wrote:

My understanding of what goes on in mergeOne is that when src is undefined, dst is lost.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/stampit-org/stamp/pull/51?email_source=notifications&email_token=AAMMEL5PFZLEW6EUGDSKBOTQRP4LXA5CNFSM4JHIVJN2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEC2PS6Q#issuecomment-548731258, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMMEL7HGIDMDKEKH4QM5TTQRP4LXANCNFSM4JHIVJNQ .

PopGoesTheWza commented 4 years ago

Do you want me to also revert the “assign() more than two parameters” fix?

koresar commented 4 years ago

Yes. I meant everything in the JS files.

On Sat., 2 Nov. 2019, 08:27 PopGoesTheWza, notifications@github.com wrote:

Do you want me to also revert the “assign() more than two parameters” fix?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/stampit-org/stamp/pull/51?email_source=notifications&email_token=AAMMELYAXWNGDGVBEZIC2ZLQRSNLRA5CNFSM4JHIVJN2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEC4GU3I#issuecomment-548956781, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMMEL34AUB2HD2K7GCEPU3QRSNLRANCNFSM4JHIVJNQ .

PopGoesTheWza commented 4 years ago

The only .js still modified in this PR is strictly for a linting issue and no code was changed.

Hence this PR addresses the CI Travis linting errors while #53 addresses the unit tests part.

koresar commented 4 years ago

Could you please create branches directly in this repo rather thank forking? I would be able to contribute to your branch with my code.

PopGoesTheWza commented 4 years ago

Could you please create branches directly in this repo rather thank forking? I would be able to contribute to your branch with my code.

@koresar Sure. But currently I can only create branches on stampit-org/stampit and not on stampit-org/stamp (or I missed something?)

> git push -u origin issue-52
remote: Permission to stampit-org/stamp.git denied to PopGoesTheWza.
fatal: unable to access 'https://github.com/stampit-org/stamp.git/': The requested URL returned error: 403
koresar commented 4 years ago

Oh. Sorry. I'll double check the permissions etc.

On Mon., 4 Nov. 2019, 18:36 PopGoesTheWza, notifications@github.com wrote:

Could you please create branches directly in this repo rather thank forking? I would be able to contribute to your branch with my code.

@koresar https://github.com/koresar Sure. But currently I can only create branches on stampit-org/stampit and not on stampit-org/stamp (or I missed something?)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/stampit-org/stamp/pull/51?email_source=notifications&email_token=AAMMEL6LJD6AFEYAKT5LTIDQR7GHJA5CNFSM4JHIVJN2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEC6NR7I#issuecomment-549247229, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMMEL25ZKZRQVAAC4FAFFLQR7GHJANCNFSM4JHIVJNQ .