search

lmammino / godaddy-dns

A Node.js script to programmatically update GoDaddy DNS records
https://lmammino.github.io/godaddy-dns/
MIT License
78 stars 21 forks source link

Potential fix for issue #26 #28

Open pjdsully opened 6 years ago

lmammino commented 6 years ago

Hey @pjdsully, any reason why you closed the PR?

I think it might be a great start. I would also love to see some test added and the doc updated.

pjdsully commented 6 years ago

Hi,

I'm new with github. The closure was a mistake; I didn't even notice it.

Truth is, I'm new with Node.js, too. I've only worked with Javascript in web content, most extensively to add VoiceOver screen reader support to Apple Help content.

So I struggled for a bit with even the structure of the project and with one specific technical issue. Notably:

  1. I don't know what doc files may be generated in whole or in part, so I shied away from modifying these.
  2. I didn't even look at tests. (Yes, I've tried the code on my own, with and without use of the switch. It doesn't check the parameter for valid IPv4 format, but works otherwise.) Having struggled to learn npm and github and Node.js in one afternoon, I didn't want to tackle whatever is needed to add tests.
  3. You suggested --force-ip. I implemented --forceip (without the hyphen). That's just because I don't know how to escape or quote the the property name that Commander evidently generates.

So that's four rookie problems. But the code does work for me, so I'm happy with it.

On Mar 7, 2018, at 4:12 PM, Luciano Mammino notifications@github.com wrote:

Hey @pjdsully https://github.com/pjdsully, any reason why you closed the PR?

I think it might be a great start. I would also love to see some test added and the doc updated.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/lmammino/godaddy-dns/pull/28#issuecomment-371269228, or mute the thread https://github.com/notifications/unsubscribe-auth/AjbTSPPrGXRheNggyLXmIiX8Ao0DqV57ks5tcD85gaJpZM4SgxaD.

This email has been scanned by the Symantec Email Security.cloud service. For more information please visit http://www.symanteccloud.com

This email has been scanned by the Symantec Email Security.cloud service. For more information please visit http://www.symanteccloud.com

pjdsully commented 6 years ago

Hello Luciano,

I remember now. I thought the "Close" button was like "OK" in a desktop application dialog.

As I said, I'm totally new with github. (I've used command line git for years, mostly with my own server.)

On Mar 7, 2018, at 4:12 PM, Luciano Mammino notifications@github.com wrote:

Hey @pjdsully https://github.com/pjdsully, any reason why you closed the PR?

I think it might be a great start. I would also love to see some test added and the doc updated.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/lmammino/godaddy-dns/pull/28#issuecomment-371269228, or mute the thread https://github.com/notifications/unsubscribe-auth/AjbTSPPrGXRheNggyLXmIiX8Ao0DqV57ks5tcD85gaJpZM4SgxaD.

This email has been scanned by the Symantec Email Security.cloud service. For more information please visit http://www.symanteccloud.com

This email has been scanned by the Symantec Email Security.cloud service. For more information please visit http://www.symanteccloud.com

lmammino commented 6 years ago

No worries @pjdsully, I'll reopen this one. If you'll ever want to spend some more time to complete it feel free to do it. I might try to spend some time on it myself as soon as I have a free weekend.

For now thanks a million for your valuable contribution, I really appreciate your effort in trying to get this done anyway!