Skip to content

refactor(rust): replace rpassword with inquire#1271

Merged
imobachgs merged 7 commits intomasterfrom
inquire
Jun 27, 2024
Merged

refactor(rust): replace rpassword with inquire#1271
imobachgs merged 7 commits intomasterfrom
inquire

Conversation

@imobachgs
Copy link
Copy Markdown
Contributor

Inquire is a library for building interactive prompts. It offers many "widgets" (check the demo!) and it looks like a better solution for our use case.

This PR replaces rpassword with Inquire.

@coveralls
Copy link
Copy Markdown

coveralls commented May 28, 2024

Coverage Status

coverage: 70.375% (-0.02%) from 70.394%
when pulling cbd5247 on inquire
into 109cead on master.

@mvidner
Copy link
Copy Markdown
Contributor

mvidner commented Jun 17, 2024

I wanted to review this and started by trying out agama auth show...

I found it quite confusing how it shows ~/.local/agama/token after I've used agama auth login but falls back to /run/agama/token (after agama auth logout), which looks the same but doesn't actually provide access: agama config show says Invalid authentication token: ExpiredSignature.

@imobachgs imobachgs modified the milestones: Agama 10, Agama 9 Jun 26, 2024
@imobachgs
Copy link
Copy Markdown
Contributor Author

I would open a new issue to improve the agama auth command. Some ideas:

  • auth login should not generate a new token if the user is already logged in (the token is valid).
  • auth show should display not only the token, but perhaps where it comes from.

Perhaps @mchf have some additional ideas.

Having said that, I think this PR is unrelated to those issues.

Copy link
Copy Markdown
Contributor

@mvidner mvidner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I don't see how this helps. 🥲
Well, the prompt ends up being colored, that is nice,
and there is a help text but I argue that the feature which is describes is superfluous.

Password::new("Please, introduce the root password:")
.with_validator(validator)
.without_confirmation()
.with_help_message("Press <esc> to exit.")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the purpose of this. It helps the user to distinguish entering an empty password from not entering a password?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I do not get the question

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main point is to switch from rpassword to inquire, which is a more general library for handling user interaction (rpassword is only for passwords).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am nitpicking on the existence of this help message. When I remove the help, I can still cancel the operation (and get a CliError::InteractivePassword).

I, as a user, am used to canceling password input by simply entering a garbage or empty password. (Unless the counterparty is a Bank that penalizes all incorrect attempts by $100 and a 3 hour delay until the next attempt, this is a work of fiction 🤣 )

For my personal UX, it is better to hide the help message, because it just draws my attention away towards a useless feature.

Copy link
Copy Markdown
Contributor

@mvidner mvidner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I still have comments but the PR is good enough for merging

@imobachgs imobachgs merged commit 841d279 into master Jun 27, 2024
@imobachgs imobachgs deleted the inquire branch June 27, 2024 12:34
@imobachgs imobachgs mentioned this pull request Jun 27, 2024
imobachgs added a commit that referenced this pull request Jun 27, 2024
Prepare for releasing Agama 9. It includes the following pull requests:

- #1101
- #1202
- #1228
- #1231
- #1236
- #1238
- #1239
- #1240
- #1242
- #1243
- #1244
- #1245
- #1246
- #1247
- #1248
- #1249
- #1250
- #1251
- #1252
- #1253
- #1254
- #1255
- #1256
- #1257
- #1258
- #1259
- #1260
- #1261
- #1264
- #1265
- #1267
- #1268
- #1269
- #1270
- #1271
- #1272
- #1273
- #1274
- #1279
- #1280
- #1284
- #1285
- #1286
- #1287
- #1288
- #1289
- #1290
- #1291
- #1292
- #1293
- #1294
- #1295
- #1296
- #1298
- #1299
- #1300
- #1301
- #1302
- #1303
- #1304
- #1305
- #1306
- #1307
- #1308
- #1309
- #1310
- #1311
- #1312
- #1313
- #1314
- #1315
- #1316
- #1317
- #1318
- #1319
- #1320
- #1321
- #1322
- #1323
- #1324
- #1325
- #1326
- #1328
- #1329
- #1331
- #1332
- #1334
- #1338
- #1340
- #1341
- #1342
- #1343
- #1344
- #1345
- #1348
- #1349
- #1351
- #1352
- #1353
- #1354
- #1355
- #1356
- #1357
- #1358
- #1359
- #1360
- #1361
- #1362
- #1363
- #1365
- #1366
- #1367
- #1368
- #1371
- #1372
- #1374
- #1375
- #1376
- #1379
- #1380
- #1381
- #1383
- #1384
- #1385
- #1386
- #1387
- #1388
- #1389
- #1391
- #1392
- #1394
- #1395
- #1397
- #1398
- #1399
- #1400
- #1403
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants