trevoro / node-named

DNS Server in node.js
MIT License
356 stars 100 forks source link

ES6 and much more #31

Closed hasezoey closed 6 years ago

hasezoey commented 6 years ago

Scripts:

.md('s):

Request i try to do npx nodeunit but always get at dnsbuffer (l51) a Range Error (with data set 2, first thing, "b9" (i dont understand much of buffers)) (and because of that i couldnt really test it) i dont know if this is a error from my editing or something that should come or so ...

Important: i couldnt test it, because of the unit test, first i need there some response, then i could test it.

trevoro commented 6 years ago

Hey @hasezoey thanks for taking the time to put this together. I'm going to close this PR for now even though I think it's a great start. Here's some feedback that makes it hard to review this.

1) Moving to ES6 for the sake of it for such a dormant project isn't really helpful. I'm more than open to adding some abstractions to the library export that might make working with ES6 easier 2) There are lots and lots of whitespace / comment / formatting changes which make the PR delta really big. As a general guideline it's a lot kinder to follow the convention of the project when submitting PRs. I know this is usually invisible to an editor and probably didn't happen on purpose, but if you look at the file diff's its kind of overwhelming.

Happy to receive feedback on code style. But this isn't a VSC project and had some other linting setup. We'll stick with that for now. 👍