Update arg parsing and default 'lounge' to 'lounge --help' #929

matthewsaunders · 2017-02-18T03:30:13Z

This PR does 2 things:

Updates lounge with no arguments to default to lounge --help.
Parse the command line arguments (fix #925).

astorije · 2017-02-18T05:01:53Z

src/command-line/start.js

@@ -13,6 +13,7 @@ program
 	.option("-B, --bind <ip>", "bind")
 	.option("    --public", "mode")
 	.option("    --private", "mode")
+  .parse(process.argv)


@xPaw, do you know why ESLint would not catch this?!

Actually, nevermind, I found the missing option and added it to #930.

@msaun008, sorry this is a nitpick, but can you replace the 2 spaces with 1 tab please? (Then use git add src/command-line/start.js && git commit --amend && git push --force to not create an extra commit)

I'm trying to understand why we need this here. I would assume the bug is happening in index.js due to incorrect order of parsing. There's program.parseOptions and program.parse calls in there.

astorije

Tried both changes and they work great, good catch @msaun008!
Asking for a second review in case I missed something obvious.

astorije · 2017-02-18T07:03:41Z

src/command-line/start.js

@@ -13,6 +13,7 @@ program
 	.option("-B, --bind <ip>", "bind")
 	.option("    --public", "mode")
 	.option("    --private", "mode")
+  .parse(process.argv)


Actually, nevermind, I found the missing option and added it to #930.

@msaun008, sorry this is a nitpick, but can you replace the 2 spaces with 1 tab please? (Then use git add src/command-line/start.js && git commit --amend && git push --force to not create an extra commit)

astorije

Sorry, I meant to approve the changes but not approve the PR because of the indentation request :)

xPaw · 2017-02-18T08:58:36Z

src/command-line/index.js

@@ -43,5 +43,5 @@ require("./edit");
 program.parse(argv.args);

 if (!program.args.length) {
-	program.parse(process.argv.concat("start"));
+	program.parse(process.argv.concat("--help"));


~~Instead of this crappy hack, can isDefault be used?~~

EDIT: Since help is automated, program.help() should be used.

I wasn't clear so I asked clarification to @xPaw and:

--help is auto-generated by commander, not something we define, so we can't use isDefault on it.

Replace program.parse(process.argv.concat("--help")); with program.help().

👍 This makes sense.

astorije · 2017-02-18T18:54:47Z

@msaun008, fix to our linter was merged in master, could you rebase with the upstream project please?

xPaw

A better fix: https://gist.github.com/xPaw/224285fcae39f426159b252b39252097

matthewsaunders · 2017-02-24T04:41:43Z

Tested the fix @xPaw suggested. This works and I agree is the better fix.

astorije

Looks good to me, thank you very much @msaun008!!
(@xPaw, I haven't tested the new version, let me know if you want me to before we can merge)

xPaw · 2017-02-24T18:41:36Z

@astorije I suspect that --home might be broken.

astorije · 2017-02-24T23:26:30Z

Argh, good catch...
Indeed:

$ node index start --port 1234 --home /tmp
2017-02-24 23:23:00 [INFO] The Lounge source (66795b0) is now running using node 6.9.4 on darwin (x64)
2017-02-24 23:23:00 [INFO] Configuration file: /Users/jeremie/.lounge/config.js
2017-02-24 23:23:00 [INFO] Available on http://0.0.0.0:1234/ in private mode
2017-02-24 23:23:00 [INFO] Press Ctrl-C to stop

And this time it' not fixed with --.
Any suggestions for fixing this, @xPaw?

Huuuuh we do need unit tests for the CLI that would have been detected :(

xPaw · 2017-02-25T07:55:17Z

Spend some time testing it, and there's really no easy way of handling global options, but here's the fix I came up with: https://gist.github.com/xPaw/224285fcae39f426159b252b39252097

The actual fix for passing options is this:

-program.parse(argv.args);
+program.parse(process.argv);

The other changes are just to improve CLI a little.

astorije · 2017-02-25T18:18:34Z

@xPaw, regarding your cleanups, why removing the config path displayed? It saved me a couple times to see that info, considering how weirdly we handle the config path.

xPaw · 2017-02-26T08:28:26Z

@astorije OK, left it as it was.

astorije · 2017-02-27T02:00:32Z

@msaun008, mind trying out the new fix suggested by @xPaw?

matthewsaunders · 2017-03-04T20:59:52Z

@xPaw Great fix. Just finished testing.

$ node index start --port 1234 --home /tmp/delme
2017-03-04 20:54:33 [INFO] Configuration file created at /tmp/delme/config.js.
2017-03-04 20:54:34 [INFO] The Lounge source (66795b0) is now running using node 4.3.1 on darwin (x64)
2017-03-04 20:54:34 [INFO] Configuration file: /tmp/delme/config.js
2017-03-04 20:54:34 [INFO] Available on http://*:1234/ in public mode
2017-03-04 20:54:34 [INFO] Press Ctrl-C to stop

astorije · 2017-03-06T23:38:26Z

Okay with this now, @xPaw? I tried to think and cannot think of another edge case we missed.

xPaw · 2017-03-08T08:00:50Z

@astorije OK. In release, document that there's -h alias now.

williamboman · 2017-03-13T21:50:47Z

This broke docker images :(. This looks like a change requiring a major bump per semver conventions imo

astorije · 2017-03-14T06:13:07Z

If this was a library I would agree, but for an end-user client application, going from v2 to v3 simply because lounge start is the new lounge (which was never documented as such, btw) it kinda extreme.
Thanks for fixing this in the Docker images!

williamboman · 2017-03-14T10:29:51Z

going from v2 to v3 simply because lounge start is the new lounge (which was never documented as such, btw) it kinda extreme.

I agree, it does feel very ridiculous to require a major bump for this - but that's what semver dictates (depending on how you define your API surface). However, I think the CLI is part of the "API surface" that should be covered by semver in the case of thelounge. I have a feeling more people launching lounge via supervisor/systemd/pm2/etc. will run into this issue when upgrading to 2.2.2.

In hindsight and imo, I think the best course of action would've been to initially introduce a deprecation notice when running lounge and make the switch in the next major release.

AlMcKinlay · 2017-03-14T10:53:29Z

but that's what semver dictates

We never agreed to follow semver.

I agree with the rest of your points (we probably should have deprecated this rather than just removing it), but for semver to mean something, we have to actually follow semver. We agreed at the beginning that we wouldn't follow semver because, as @astorije says, we are an end user application.

williamboman · 2017-03-14T10:55:10Z

We never agreed to follow semver.

Oh, I thought we adhered to it. That makes my point less valid 😄

AlMcKinlay · 2017-03-14T11:06:14Z

Huh, I just noticed that @astorije put that we follow semver in the CHANGELOG, but from this irc conversation last year, I thought we had agreed we wouldn't. But maybe that was my assumption and no one else agreed.

[2016-04-10 23:16:48] <astorije> Impaloo, I don't think it will, as it doesn't involve any breaking changes, per the semver document
[2016-04-10 23:17:40] <astorije> In fact, I don't believe we'll go to a v2 before any public API gets involved in The Lounge (like a package system for example)
[2016-04-10 23:17:47] <Impaloo> astorije: I feel like semver + client applications makes for a weird combination
[2016-04-10 23:18:57] <astorije> That's true, it will be more relevant when such API is implemented
[2016-04-10 23:19:10] <Impaloo> Or maybe not, what else is there to base versioning on
[2016-04-11 07:38:57] <xPaw> I would say 2.0 is warranted if we flip on reconnections after merging irc-fw
[2016-04-11 07:39:08] <xPaw> following semver to the stongest point of "breaking changes" is a bit silly
[2016-04-11 07:50:15] <YaManicKill> I think semver for software only used by a person does seem silly. Once we have packages, if we break the package api then we should use semver, but until then, , I think we switch to 2.0 when we have a number of good features, not just 1.
[2016-04-11 07:52:05] <xPaw> I'd say a major internal change (basically switching out entire IRC parser and adding ircv3) can be breaking
[2016-04-11 08:16:21] <YaManicKill> Breaking for who?
[2016-04-11 08:25:48] * hardtheory join
[2016-04-11 08:28:33] <xPaw> clients? there's always a chance there's an obscure server that will do something weird
[2016-04-11 08:44:27] <YaManicKill> But the point of semver isn't "We might have introduced bugs" but "This api has changed and will not work the same as before"
[2016-04-11 08:44:37] <YaManicKill> Ours is very definitely the second.
[2016-04-11 08:46:24] <xPaw> we have no api
[2016-04-11 08:46:47] <xPaw> and following your logic, we will be stuck in 1.x forever
[2016-04-11 08:50:44] <lpoujol> Well, there's the internal API. Anyone who have thinkered in the code will probably have broken stuff after the update to irc-fw
[2016-04-11 08:54:08] * FortuneDays action  tinkered with the code
[2016-04-11 08:55:33] <lpoujol> p
[2016-04-11 09:31:41] <YaManicKill> xPaw: no, my logic is that we don't follow semver, but when we feel we have a few good features to distinguish us from shout, then we can bump.
[2016-04-11 09:32:01] <YaManicKill> lpoujol: well yeah, but by that logic we could be breaking stuff every time we add or remove anything.
[2016-04-11 09:32:14] <YaManicKill> At what point do we say that it's enough to be breaking?
[2016-04-11 09:33:39] <lpoujol> I would say "could break a lot" so.
[2016-04-11 09:33:55] <YaManicKill> Also, I know we have no API. That's exactly the point. Semver is for things with APIs.
[2016-04-11 09:34:13] <YaManicKill> http://semver.org/ "MAJOR version when you make incompatible API changes,"
[2016-04-11 09:34:13] *  toggle
[2016-04-11 09:34:39] <YaManicKill> "MAJOR version when you make incompatible API changes,"
[2016-04-11 09:34:41] <YaManicKill> Ah wrong copy
[2016-04-11 09:35:13] <CornishPasty> I do think it's something worth bumping major for
[2016-04-11 09:35:55] <lpoujol> Oh
[2016-04-11 09:36:07] <CornishPasty> Even without semver
[2016-04-11 09:36:09] <lpoujol> For this system to work, you first need to declare a public API
[2016-04-11 09:36:31] <lpoujol> No public API -> You can do whatever you want
[2016-04-11 09:36:38] <lpoujol> It's the case, right?
[2016-04-11 09:36:41] <CornishPasty> Is lounge getting a plugin framework any time soon?
[2016-04-11 09:36:59] <xPaw> no
[2016-04-11 09:37:14] <YaManicKill> "For this system to work, you first need to declare a public API."
[2016-04-11 09:37:24] <YaManicKill> lpoujol: yes, exactly
[2016-04-11 09:37:30] <YaManicKill> CornishPasty: now, that's a different discussion
[2016-04-11 09:37:47] <YaManicKill> IF we think that irc framework is enough for 2.0.0 (I don't), then we can discuss that.
[2016-04-11 09:38:00] <YaManicKill> But if we are talking about whether we follow semver or not, the answer is we can't.

astorije · 2017-03-15T04:38:12Z

I agree with both of you but I think we are overthinking this a little bit :)
AFAIK the only documented way to start a server is through lounge start, and this is true since our first release. The fact that lounge also started the server is not documented anywhere.

If we had moved starting a server from lounge to lounge start, deprecation + transition time would be in order indeed. But this release does not break lounge start, it makes it less dramatic lounge: showing help instead of starting a full server.

Also, considering our currently... ahem, questionable CLI, it's likely that a lot of changes would/will be breaking stuff :)
If we were designing a library, things would be very different as libraries are consumed by other software. In our case, our CLI is mostly consumed by humans, who should read the first lines of the release notes before upgrading at the very least 😅.

I am fine to remove This project adheres to Semantic Versioning in the changelog if you want. It looks like we all agree on the fact that we are following a sorta-semver for end-user applications and that things are less black and white there. This is really not the kind of things I want to focus on 😄.

…ine-arg-parsing Update arg parsing and default 'lounge' to 'lounge --help'

matthewsaunders mentioned this pull request Feb 18, 2017

CLI options are ignored when given in a specific order #925

Closed

astorije reviewed Feb 18, 2017

View reviewed changes

astorije self-requested a review February 18, 2017 06:30

astorije added the Type: Bug Issues that report and PRs that solve any defects that cause unexpected behaviors. label Feb 18, 2017

astorije added this to the 2.2.2 milestone Feb 18, 2017

astorije mentioned this pull request Feb 18, 2017

Make sure multiline chains of calls are correctly indented #930

Merged

astorije approved these changes Feb 18, 2017

View reviewed changes

astorije requested changes Feb 18, 2017

View reviewed changes

astorije requested a review from xPaw February 18, 2017 07:05

xPaw reviewed Feb 18, 2017

View reviewed changes

xPaw requested changes Feb 18, 2017

View reviewed changes

matthewsaunders force-pushed the msaun008/fix-command-line-arg-parsing branch 2 times, most recently from fd73296 to 66795b0 Compare February 24, 2017 04:25

astorije approved these changes Feb 24, 2017

View reviewed changes

xPaw closed this Feb 24, 2017

xPaw reopened this Feb 24, 2017

Update arg parsing and default 'lounge' to 'lounge --help'

86ed0b6

matthewsaunders force-pushed the msaun008/fix-command-line-arg-parsing branch from 66795b0 to 86ed0b6 Compare March 4, 2017 20:59

xPaw approved these changes Mar 8, 2017

View reviewed changes

xPaw merged commit 24bd393 into thelounge:master Mar 10, 2017

astorije mentioned this pull request Mar 13, 2017

Fix -h help option alias shadowed by a similar alias for --home #959

Merged

matburnham pushed a commit to matburnham/lounge that referenced this pull request Sep 6, 2017

Merge pull request thelounge#929 from msaun008/msaun008/fix-command-l…

1282bce

…ine-arg-parsing Update arg parsing and default 'lounge' to 'lounge --help'

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update arg parsing and default 'lounge' to 'lounge --help' #929

Update arg parsing and default 'lounge' to 'lounge --help' #929

matthewsaunders commented Feb 18, 2017

astorije Feb 18, 2017

astorije Feb 18, 2017

xPaw Feb 18, 2017

astorije left a comment

astorije Feb 18, 2017

astorije left a comment

xPaw Feb 18, 2017 •

edited

Loading

astorije Feb 18, 2017

matthewsaunders Feb 18, 2017

astorije commented Feb 18, 2017

xPaw left a comment

matthewsaunders commented Feb 24, 2017

astorije left a comment

xPaw commented Feb 24, 2017

astorije commented Feb 24, 2017

xPaw commented Feb 25, 2017 •

edited

Loading

astorije commented Feb 25, 2017

xPaw commented Feb 26, 2017

astorije commented Feb 27, 2017

matthewsaunders commented Mar 4, 2017

astorije commented Mar 6, 2017

xPaw commented Mar 8, 2017

williamboman commented Mar 13, 2017

astorije commented Mar 14, 2017

williamboman commented Mar 14, 2017 •

edited

Loading

AlMcKinlay commented Mar 14, 2017

williamboman commented Mar 14, 2017

AlMcKinlay commented Mar 14, 2017

astorije commented Mar 15, 2017

Update arg parsing and default 'lounge' to 'lounge --help' #929

Update arg parsing and default 'lounge' to 'lounge --help' #929

Conversation

matthewsaunders commented Feb 18, 2017

astorije Feb 18, 2017

Choose a reason for hiding this comment

astorije Feb 18, 2017

Choose a reason for hiding this comment

xPaw Feb 18, 2017

Choose a reason for hiding this comment

astorije left a comment

Choose a reason for hiding this comment

astorije Feb 18, 2017

Choose a reason for hiding this comment

astorije left a comment

Choose a reason for hiding this comment

xPaw Feb 18, 2017 • edited Loading

Choose a reason for hiding this comment

astorije Feb 18, 2017

Choose a reason for hiding this comment

matthewsaunders Feb 18, 2017

Choose a reason for hiding this comment

astorije commented Feb 18, 2017

xPaw left a comment

Choose a reason for hiding this comment

matthewsaunders commented Feb 24, 2017

astorije left a comment

Choose a reason for hiding this comment

xPaw commented Feb 24, 2017

astorije commented Feb 24, 2017

xPaw commented Feb 25, 2017 • edited Loading

astorije commented Feb 25, 2017

xPaw commented Feb 26, 2017

astorije commented Feb 27, 2017

matthewsaunders commented Mar 4, 2017

astorije commented Mar 6, 2017

xPaw commented Mar 8, 2017

williamboman commented Mar 13, 2017

astorije commented Mar 14, 2017

williamboman commented Mar 14, 2017 • edited Loading

AlMcKinlay commented Mar 14, 2017

williamboman commented Mar 14, 2017

AlMcKinlay commented Mar 14, 2017

astorije commented Mar 15, 2017

xPaw Feb 18, 2017 •

edited

Loading

xPaw commented Feb 25, 2017 •

edited

Loading

williamboman commented Mar 14, 2017 •

edited

Loading