search

octachrome / treason

A clone of the card game Coup written in Node.js
Other
137 stars 79 forks source link

`command` is an unwieldy function #35

Open dnathe4th opened 4 years ago

dnathe4th commented 4 years ago

In my fork I have taken some time to refactor command to be a bit more manageable, with clearer mapping between valid commands and states, as well as deduplication of some of the validation-type checks in each function.

Would you entertain a refactor like this? https://github.com/dnathe4th/treason/commit/5ce8493407e1b4e28a79e5a6f17acfba413da82e#diff-41c4530a42496f695607188d7bbe06d5

octachrome commented 4 years ago

First I feel the need to apologise for my code in this file. It is an ugly mess long overdue refactoring. Thanks for taking the time. If you are willing to create a pull request then I would have some comments/suggestions and we could get this merged. It would be good to include this commit too: https://github.com/dnathe4th/treason/commit/b5140124a2f772ccade866b7d224a744b098fbb7.

dnathe4th commented 4 years ago

Happily. Let me cherry-pick it off my fork after work today.

I'm new-ish to OSS licensing. I created an amendment in my fork for my work, but how should that look once it gets merged into the main project?

On Wed, Apr 8, 2020 at 4:29 AM Chris Brown notifications@github.com wrote:

First I feel the need to apologise for my code in this file. It is an ugly mess long overdue refactoring. Thanks for taking the time. If you are willing to create a pull request then I would have some comments/suggestions and we could get this merged. It would be good to include this commit too: dnathe4th@b514012 https://github.com/dnathe4th/treason/commit/b5140124a2f772ccade866b7d224a744b098fbb7 .

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/octachrome/treason/issues/35#issuecomment-610903991, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACKQ7TL3QP4IHU6GAPTIVDRLRNZBANCNFSM4MBSBILA .

octachrome commented 4 years ago

Good question. In your license amendments you have claimed copyright/ownership on your modifications, which is your right: https://softwareengineering.stackexchange.com/questions/211365/who-owns-modifications-to-open-source-software. I think I would prefer for you to transfer ownership of your contributions to me upon merging, to avoid losing my ability to do what I want with the project as a whole (otherwise I would have to get your permission on everything or revert your changes). I would like to add a list of contributors to the license file, where you would be credited. How do you feel about that?

dnathe4th commented 4 years ago

That sounds good to me

On Wed, Apr 8, 2020 at 10:26 AM Chris Brown notifications@github.com wrote:

Good question. In your license amendments you have claimed copyright/ownership on your modifications, which is your right: https://softwareengineering.stackexchange.com/questions/211365/who-owns-modifications-to-open-source-software. I think I would prefer for you to transfer ownership of your contributions to me upon merging, to avoid losing my ability to do what I want with the project as a whole (otherwise I would have to get your permission on everything or revert your changes). I would like to add a list of contributors to the license file, where you would be credited. How do you feel about that?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/octachrome/treason/issues/35#issuecomment-611087963, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACKQ7WRIKN32O4Y6MKURP3RLSXT7ANCNFSM4MBSBILA .

dnathe4th commented 4 years ago

As long as you're ok taking ownership of my bad code :- )

On Wed, Apr 8, 2020 at 10:50 AM Dom Narducci dnathe4th@gmail.com wrote:

That sounds good to me

On Wed, Apr 8, 2020 at 10:26 AM Chris Brown notifications@github.com wrote:

Good question. In your license amendments you have claimed copyright/ownership on your modifications, which is your right: https://softwareengineering.stackexchange.com/questions/211365/who-owns-modifications-to-open-source-software. I think I would prefer for you to transfer ownership of your contributions to me upon merging, to avoid losing my ability to do what I want with the project as a whole (otherwise I would have to get your permission on everything or revert your changes). I would like to add a list of contributors to the license file, where you would be credited. How do you feel about that?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/octachrome/treason/issues/35#issuecomment-611087963, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACKQ7WRIKN32O4Y6MKURP3RLSXT7ANCNFSM4MBSBILA .

octachrome commented 4 years ago

Ok cool. If you wouldn't mind updating the license before merging then, remove your copyright and add yourself as a contributor instead. If you want to itemize your contributions that's fine.

dnathe4th commented 4 years ago

Yeah need to do a bunch of clean up, too much going on on my own branches. Later today.

On Wed, Apr 8, 2020 at 11:02 AM Chris Brown notifications@github.com wrote:

Ok cool. If you wouldn't mind updating the license before merging then, remove your copyright and add yourself as a contributor instead. If you want to itemize your contributions that's fine.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/octachrome/treason/issues/35#issuecomment-611105724, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACKQ7VEJPQOPIATAHWT6UDRLS3Z7ANCNFSM4MBSBILA .