Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dns: add missing exports.BADNAME #3051

Closed
wants to merge 1 commit into from

Conversation

silverwind
Copy link
Contributor

Fixes #3050. Looks to be an mistake in ecfe32e.

@silverwind silverwind added the dns Issues and PRs related to the dns subsystem. label Sep 24, 2015
@silverwind silverwind added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 24, 2015
@evanlucas
Copy link
Contributor

Yea, I would say this is semver-major for sure.

@silverwind
Copy link
Contributor Author

Reconsidered the semver thing, I'll make it major just in case.

@evanlucas
Copy link
Contributor

https://github.com/search?l=javascript&q=dns.ADNAME&type=Code&utf8=%E2%9C%93 shows some uses of it in packages. OTOH, it is documented as being dns.BADNAME

@ChALkeR
Copy link
Member

ChALkeR commented Sep 24, 2015

For 5.x, this should remove EADNAME and add EBADNAME (semver-major).
For 4.x, this should just add EBADNAME, without removing EADNAME (semver-minor or even a patch, because it was documented).

@silverwind
Copy link
Contributor Author

That looks like one public use case. I think it's fine if we target this to 5.0.0, it took people 3,5 years to notice that mistake, so can't be this urgent :)

@ChALkeR
Copy link
Member

ChALkeR commented Sep 24, 2015

This list is partial (≈30%) and quite old (3⅔ months):

EADNAME:

c/cares-1.0.1.tgz/lib/cares.js:381:exports.ADNAME = 'EADNAME';

EBADNAME:

g/google-closure-compiler-20150505.0.0.tgz/contrib/nodejs/dns.js:146:dns.BADNAME = 'EBADNAME';
t/tftp-0.1.1.tgz/lib/protocol/errors.js:50:define ("EBADNAME", "Invalid filename");
t/tftp-0.1.1.tgz/lib/protocol/packets/read-request.js:14:               throw errors.EBADNAME;

Full and updated data file is still building (25% done).

Looks that ADNAME isn't really used at least by module authors.

Updated (searched over those 25%) :

aqua-1.0.0.tgz/externs/nodejs/dns.js:144:dns.BADNAME = 'EBADNAME';
biojs-vis-blast-0.1.5.tgz/node/lib/dns.js:323:exports.ADNAME = 'EADNAME';
cares-1.0.1.tgz/lib/cares.js:381:exports.ADNAME = 'EADNAME';
chromify-0.0.2.tgz/builtins/dns.js:211:exports.BADNAME = 'EBADNAME';
closurecompiler-externs-1.0.4.tgz/dns.js:144:dns.BADNAME = 'EBADNAME';
commonjs-everywhere-0.9.7.tgz/node/lib/dns.js:203:exports.ADNAME = 'EADNAME';
dnsjs-0.1.1.tgz/lib/index.js:37:    this.BADNAME = consts.BADNAME;
dnsjs-0.1.1.tgz/lib/index.js:286:    this.BADNAME = consts.BADNAME;
dnsjs-0.1.1.tgz/mocha/tests.js:39:    this.BADNAME = consts.BADNAME;
dnsjs-0.1.1.tgz/mocha/tests.js:288:    this.BADNAME = consts.BADNAME;
dnsjs-0.1.1.tgz/mocha/tests.js:14071:  BADNAME: 20,
dnsjs-0.1.1.tgz/mocha/tests.js:14077:exports.BADNAME = 'EBADNAME';
e2e-0.0.6.tgz/end-to-end/lib/closure-compiler/contrib/nodejs/dns.js:146:dns.BADNAME = 'EBADNAME';
flush-all-0.1.1.tgz/node-v0.13/lib/dns.js:323:exports.ADNAME = 'EADNAME';

Edit: updated a bit.

@silverwind
Copy link
Contributor Author

Thanks for that analyses @ChALkeR, much appreciated.

@thefourtheye
Copy link
Contributor

For 5.x, this should remove EADNAME and add EBADNAME (semver-major).
For 4.x, this should just add EBADNAME, without removing EADNAME (semver-minor or even a patch, because it was documented).

I agree with @ChALkeR. Probably, we can introduce the documentation entry for ADNAME in 4.x and indicate that this will be fixed in 5.x?

@silverwind
Copy link
Contributor Author

I don't really see a benefit in documenting the missspelled property. I think I'll go with a patch commit for the addition of the value and a semver-major for the removal of the wrong value.

@ChALkeR
Copy link
Member

ChALkeR commented Sep 25, 2015

@silverwind

I don't really see a benefit in documenting the missspelled property. I think I'll go with a patch commit for the addition of the value and a semver-major for the removal of the wrong value.

+1

@ChALkeR
Copy link
Member

ChALkeR commented Sep 26, 2015

And the updated (2015-09-21) and more complete (all the current version of packages, except for the minified code) list is here.

Everything that includes ADNAME (with BADNAME, EADNAME, and EBADNAME) :

aqua-1.0.0.tgz/externs/nodejs/dns.js:144:dns.BADNAME = 'EBADNAME';
biojs-vis-blast-0.1.5.tgz/node/lib/dns.js:323:exports.ADNAME = 'EADNAME';
cares-1.0.1.tgz/lib/cares.js:381:exports.ADNAME = 'EADNAME';
chromify-0.0.2.tgz/builtins/dns.js:211:exports.BADNAME = 'EBADNAME';
closurecompiler-externs-1.0.4.tgz/dns.js:144:dns.BADNAME = 'EBADNAME';
commonjs-everywhere-0.9.7.tgz/node/lib/dns.js:203:exports.ADNAME = 'EADNAME';
dnsjs-0.1.1.tgz/lib/index.js:37:    this.BADNAME = consts.BADNAME;
dnsjs-0.1.1.tgz/lib/index.js:286:    this.BADNAME = consts.BADNAME;
dnsjs-0.1.1.tgz/mocha/tests.js:39:    this.BADNAME = consts.BADNAME;
dnsjs-0.1.1.tgz/mocha/tests.js:288:    this.BADNAME = consts.BADNAME;
dnsjs-0.1.1.tgz/mocha/tests.js:14071:  BADNAME: 20,
dnsjs-0.1.1.tgz/mocha/tests.js:14077:exports.BADNAME = 'EBADNAME';
e2e-0.0.6.tgz/end-to-end/lib/closure-compiler/contrib/nodejs/dns.js:146:dns.BADNAME = 'EBADNAME';
flush-all-0.1.1.tgz/node-v0.13/lib/dns.js:323:exports.ADNAME = 'EADNAME';
google-closure-compiler-20150901.0.0.tgz/contrib/nodejs/dns.js:146:dns.BADNAME = 'EBADNAME';
Haraka-2.6.1.tgz/outbound.js:807:    // BADNAME
jsg-0.0.3.tgz/testdata/node_core_modules/dns.js:258:exports.ADNAME = 'EADNAME';
native-dns-0.7.0.tgz/dns.js:42:exports.BADNAME = consts.BADNAME;
native-dns-packet-0.1.1.tgz/consts.js:151:  BADNAME: 20,
native-dns-packet-0.1.1.tgz/consts.js:157:exports.BADNAME = 'EBADNAME';
nice-http-0.1.0-alfa.tgz/src/dns.js:256:exports.ADNAME = 'EADNAME';
nmcns-0.1.3.tgz/mocha/tests.js:13489:    this.BADNAME = consts.BADNAME;
nmcns-0.1.3.tgz/mocha/tests.js:13738:    this.BADNAME = consts.BADNAME;
nmcns-0.1.3.tgz/mocha/tests.js:31711:  BADNAME: 20,
nmcns-0.1.3.tgz/mocha/tests.js:31717:exports.BADNAME = 'EBADNAME';
node-core-lib-0.11.11.tgz/dns.js:258:exports.ADNAME = 'EADNAME';
nodedbi-1.0.7.tgz/index.js:9:  DBI_ERROR_BADNAME,
nodedbi-1.0.7.tgz/index.js:23:nodedbi['DBI_ERROR_BADNAME'] = -5;
nodejs-externs-0.10.1.tgz/externs/dns.js:144:dns.BADNAME = 'EBADNAME';
node-natives-0.10.25.tgz/dns.js:203:exports.ADNAME = 'EADNAME';
pezhu-0.0.0.tgz/Downloads/node-v0.9.11/lib/dns.js:202:exports.ADNAME = 'EADNAME';
pn-0.0.1.tgz/dns.js:7:  ADNAME: { enumerable: true, value: dns.ADNAME },
portable-js-0.0.3.tgz/misc/io/dns.js:338:exports.ADNAME = 'EADNAME';
portable-js-0.0.3.tgz/misc/node/dns.js:325:exports.ADNAME = 'EADNAME';
sawrocket-xmpp-0.3.0.tgz/sawrocket-xmpp.browser.js:14164:exports.BADNAME = consts.BADNAME;
sawrocket-xmpp-0.3.0.tgz/sawrocket-xmpp.browser.js:17115:  BADNAME: 20,
sawrocket-xmpp-0.3.0.tgz/sawrocket-xmpp.browser.js:17121:exports.BADNAME = 'EBADNAME';
smb2-0.2.6.tgz/lib/tools/ms_erref.js:17712:    "code":"ERROR_SXS_XML_E_BADNAMECHAR"
srb-0.1.14.tgz/lib/tasks/dns.js:14:dns.BADNAME: Misformatted domain name.
tftp-0.1.2.tgz/lib/protocol/errors.js:50:define ("EBADNAME", "Invalid filename");
tftp-0.1.2.tgz/lib/protocol/packets/read-request.js:14:                throw errors.EBADNAME;

@silverwind silverwind changed the title dns: add missing dns.BADNAME in place of dns.ADNAME dns: add missing exports.EBADNAME Sep 26, 2015
@silverwind silverwind removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 26, 2015
@silverwind
Copy link
Contributor Author

Changed this PR to just add the missing EBADNAME. The part which removes EADNAME is at #3076. PTAL.

@targos
Copy link
Member

targos commented Sep 26, 2015

LGTM

@silverwind silverwind changed the title dns: add missing exports.EBADNAME dns: add missing exports.BADNAME Sep 26, 2015
@silverwind
Copy link
Contributor Author

Fixed the commit title.

@ChALkeR
Copy link
Member

ChALkeR commented Sep 26, 2015

LGTM.

This is not a semver-minor, but a patch, because it's actually a bugfix (dns.BADNAME is documented but actually missing atm).

@ChALkeR
Copy link
Member

ChALkeR commented Sep 26, 2015

@silverwind Could you add that to the commit message details, btw (the fact that it was documented)?

@silverwind
Copy link
Contributor Author

Yeah, will add a description and links to the commit when I land this tomorrow.

@evanlucas
Copy link
Contributor

LGTM

silverwind added a commit that referenced this pull request Sep 27, 2015
Adds the documented but missing DNS error exports.BADNAME. This export
has been there before but got lost in a 2012 commit that added more
error codes. #3076 will remove the
wrong error code exports.ADNAME.

PR-URL: #3051
Fixes: #3050
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
@silverwind silverwind closed this Sep 27, 2015
silverwind added a commit to silverwind/node that referenced this pull request Sep 27, 2015
This error code export was mistakingly introduced in a 2012 commit which
added more error codes. The correct export.BADNAME was added in
nodejs#3051.

Semver: Major
PR-URL: nodejs#3051
Fixes: nodejs#3050
silverwind added a commit that referenced this pull request Sep 27, 2015
This error code export was mistakingly introduced in a 2012 commit which
added more error codes. The correct export.BADNAME was added in
#3051.

Semver: Major
PR-URL: #3051
Fixes: #3050
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
silverwind added a commit that referenced this pull request Sep 30, 2015
Adds the documented but missing DNS error exports.BADNAME. This export
has been there before but got lost in a 2012 commit that added more
error codes. #3076 will remove the
wrong error code exports.ADNAME.

PR-URL: #3051
Fixes: #3050
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
silverwind added a commit that referenced this pull request Sep 30, 2015
This error code export was mistakingly introduced in a 2012 commit which
added more error codes. The correct export.BADNAME was added in
#3051.

Semver: Major
PR-URL: #3051
Fixes: #3050
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
This was referenced Sep 30, 2015
@rvagg rvagg mentioned this pull request Dec 17, 2015
MylesBorins pushed a commit that referenced this pull request Jan 14, 2016
This error code export was mistakingly introduced in a 2012 commit which
added more error codes. The correct export.BADNAME was added in
#3051.

Semver: Major
PR-URL: #3051
Fixes: #3050
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 19, 2016
This error code export was mistakingly introduced in a 2012 commit which
added more error codes. The correct export.BADNAME was added in
#3051.

Semver: Major
PR-URL: #3051
Fixes: #3050
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dns Issues and PRs related to the dns subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants