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

udp: remove ancient check #8088

Merged
merged 1 commit into from
Aug 13, 2016
Merged

Conversation

saghul
Copy link
Member

@saghul saghul commented Aug 12, 2016

Unix datagram support was removed 5 years ago.

@nodejs-github-bot nodejs-github-bot added the dgram Issues and PRs related to the dgram subsystem / UDP. label Aug 12, 2016
@Trott
Copy link
Member

Trott commented Aug 12, 2016

LGTM if CI is green (which it ought to be as the text unix_dgram doesn't appear anywhere in the tests).

Is this technically semver-major on the "changing an error message is a semver major change" grounds?

@addaleax addaleax added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 12, 2016
@addaleax
Copy link
Member

LGTM

I guess, yes. I also don’t see any reason why labelling this semver-major would be a bad idea, so I’m adding the label.

@ChALkeR
Copy link
Member

ChALkeR commented Aug 13, 2016

Search, minus node source copies and minus tests:

ain2-2.0.0.tgz/index.js:131:        var socket = getSocket('unix_dgram');
ain2-fs-0.0.4.tgz/index.js:44:            var client = dgram.createSocket('unix_dgram') ;
ain2-papandreou-0.3.1.tgz/index.js:230:                        this.unixDatagramSocket = unixDgram.createSocket('unix_dgram');
cloudwatchd-0.4.0.tgz/backends/syslog.js:50:  return type == 'unix' ? 'unix_dgram' : 'UDP'
log4js-syslog-appender-0.3.0.tgz/lib/log4js-syslog-appender.js:49:        logger = new syslog({transport: "unix_dgram", tag: tag, facility: facility, path: path});
metrics-broker-0.2.1.tgz/broker/broker.js:201:  self.broker = dgram.createSocket('unix_dgram');
metrics-broker-0.2.1.tgz/broker/client.js:10:var client = dgram.createSocket('unix_dgram');
monitr-0.2.0.tgz/examples/i_am_monitoring.js:17:var monitorSocket = dgram.createSocket('unix_dgram');
nice-http-0.1.0-alfa.tgz/src/dgram.js:71:  if (type == 'unix_dgram')
node-nativesyslog-1.0.1.tgz/lib/node-syslog.js:39:      this.socket = dgram.createSocket("unix_dgram");
node_wpa_cli-1.3.8.tgz/index.js:25:    this.client = unix.createSocket('unix_dgram')
nx-0.8.9.tgz/lib/nx/server/log/writer/Syslog/write.js:19:    var client = dgram.createSocket("unix_dgram");
process-watcher-0.0.2.tgz/lib/index.js:67:    monitorSocket = dgram.createSocket('unix_dgram');
sawrocket-node-0.4.0.tgz/dgram.js:81:  if (type == 'unix_dgram')
sawrocket-xmpp-0.3.0.tgz/sawrocket-xmpp.browser.js:27575:  if (type == 'unix_dgram')
snort-socket-0.0.22a.tgz/lib/listener.js:68:                    socket = dgram.createSocket('unix_dgram');
systemd-daemon-1.0.4.tgz/lib/notify.js:21:      var client = unix.createSocket('unix_dgram');
unix-dgram-1.0.0.tgz/lib/unix_dgram.js:48:  if (type != 'unix_dgram')
unix-dgram-papandreou-0.0.1.tgz/src/unix_dgram.js:50:  if (type != 'unix_dgram')
unix-socket-streams2-1.1.0.tgz/index.js:118:                socket = unix.createSocket('unix_dgram');
winston-syslog-1.2.1.tgz/lib/winston-syslog.js:265:      this.socket = require('unix-dgram').createSocket('unix_dgram');
winston-syslog-1.2.1.tgz/lib/winston-syslog.js:358:  this.socket = require('unix-dgram').createSocket('unix_dgram');
wpa_state-1.3.0.tgz/index.js:26:  this.client = unix.createSocket('unix_dgram')
wpa-wifi-0.2.2.tgz/index.js:40:            this.client = unix.createSocket('unix_dgram');

LGTM.

@mscdex
Copy link
Contributor

mscdex commented Aug 13, 2016

LGTM

1 similar comment
@JungMinu
Copy link
Member

LGTM

@bnoordhuis
Copy link
Member

LGTM. Some of the modules in @ChALkeR's list are using (and one of them is) my node-unix-dgram module so the actual list is even smaller.

PR-URL: nodejs#8088
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
@saghul saghul closed this Aug 13, 2016
@saghul saghul deleted the remove-unix-dgram-check branch August 13, 2016 07:39
@saghul saghul merged commit 6a3dbda into nodejs:master Aug 13, 2016
@Trott
Copy link
Member

Trott commented Aug 13, 2016

If I'm reading everything correctly, this didn't get a CI run.

After the fact CI: https://ci.nodejs.org/job/node-test-pull-request/3654/

@saghul
Copy link
Member Author

saghul commented Aug 14, 2016

@Trott I skipped the CI in this case, because the option wasn't used anywhere in the code, for the past 5 years.

@Trott
Copy link
Member

Trott commented Aug 15, 2016

@saghul IMO it's a good idea to run it anyway. It sets a good example for newer contributors, among other things. (But yes, it's difficult to imagine a scenario where a code change like this would blow up in CI if tests passed locally for you.)

@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dgram Issues and PRs related to the dgram subsystem / UDP. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants