Rewrite for new BetterDiscord beta #120
Conversation
betterdiscordctl
Outdated
| if [[ ${2+x} ]]; then bd_repo_branch=$2; shift | ||
| else die_non_empty '--bd-repo-branch'; fi | ||
| -b|--betterdiscord) | ||
| if [[ ${2+x} ]]; then asar=$2; shift |
There was a problem hiding this comment.
Flag names should mirror variable names. Either rename the variable asar to betterdiscord, rename --betterdiscord to --asar, or rename both to something more descriptive. If renaming, --bd-asar-file & bd_asar_file feels good for its semantic complexity. I don't know what a good short option name would be, but we could drop the short option. We should probably rename the option from --betterdiscord given the semantic change anyways (specifying overall directory → specifying asar file).
There was a problem hiding this comment.
I think -a/--bd-asar may be better since we're using -r/--bd-repo.
|
@ObserverOfTime Requesting review of my changes. |
|
Also, thank you for picking this up! |
bb010g
force-pushed
the
beta
branch
2 times, most recently
from
d7fade4
to
d14efba
Compare
There was a problem hiding this comment.
Reviewed 2 of 2 files at r3.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @bb010g and @ObserverOfTime)
betterdiscordctl, line 301 at r1 (raw file):
Previously, ObserverOfTime wrote…
Using
-f ''to install to stable does not work and requires-f ':'. This change fixes that.
This was properly solved by changing flavors parsing to IFS=':' read -ra flavors <<< "$2:", as read drops the trailing separator. With that, ${flavors[@]} is always correct.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @ObserverOfTime)
betterdiscordctl, line 119 at r1 (raw file):
Previously, ObserverOfTime wrote…
I think
-a/--bd-asarmay be better since we're using-r/--bd-repo.
Switching to --bd-asar would be fine, but do you think the option deserves to also be short? --betterdiscord had the benefit of working off of a plain Git repository, but --bd-asar works off of a build product. I think leaving -a open for other options would be better, as this is a more niche / dev-oriented flag?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @ObserverOfTime)
betterdiscordctl, line 32 at r3 (raw file):
Previously, ObserverOfTime wrote…
This can't actually be defined here because if
--bd-asar-file(which I want to rename to--bd-asar) is defined to another location, and flatpak or snap are not used, the asar file will not there.
As defaults, these values are okay. We set xdg_config but reassign it later in both bdc_flatpak & bdc_snap, which may be called from bdc_main. We can do the same for bd_config & bd_asar.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @bb010g and @ObserverOfTime)
betterdiscordctl, line 189 at r3 (raw file):
Previously, ObserverOfTime wrote…
I don't think there's a point in checking for links when we're not explicitly using them.
This is mostly trying to help with future debugging in case of jank setups. When someone posts betterdiscordctl status output in an issue, we should know if this is a link at all.
|
betterdiscordctl, line 119 at r1 (raw file): Previously, bb010g (bb010g) wrote…
It should be fine as we can always use |
bb010g
force-pushed
the
beta
branch
2 times, most recently
from
7bf4273
to
f093cfb
Compare
|
betterdiscordctl, line 32 at r3 (raw file): Previously, bb010g (bb010g) wrote…
|
There was a problem hiding this comment.
Reviewed 1 of 2 files at r4, 1 of 1 files at r5.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @bb010g and @ObserverOfTime)
betterdiscordctl, line 32 at r3 (raw file):
Previously, ObserverOfTime wrote…
- Both of them are defined using the value of
xdg_configbefore it is reassigned.betterdiscord.asarmay not be inbd_configif the user specifies another file.
Oh, we only copy in on Flatpak & Snap. That changes things. I want to bring back -c to control this?
betterdiscordctl, line 285 at r3 (raw file):
Previously, ObserverOfTime wrote…
Let's keep this error message in a single line.
[[ -d $modules ]] || die 'ERROR: Discord modules directory not found. Try specifying it with "--modules".'
This follows our pattern from (the few) other multi-sentence stderr prints we have, mainly our argument parsing dies.
betterdiscordctl, line 298 at r3 (raw file):
Previously, ObserverOfTime wrote…
I don't think there's a reason to keep the warning.
These warnings are (currently) one of the main ways to see which config directory actually gets chosen.
betterdiscordctl, line 377 at r3 (raw file):
Previously, ObserverOfTime wrote…
Maybe rename to
escaped_asarand use single quotes?declare escaped_asar=${bd_asar/\\/\\\\} printf $'require(\'%s\'); module.exports = require(\'./core.asar\'); ' "${escaped_asar//$'\''/\\$'\''}" > "$core/index.js"
Double quotes on the require here matches Zere's syntax, which I do care about maintaining on significant patched lines.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r6.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @bb010g and @ObserverOfTime)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @bb010g and @ObserverOfTime)
betterdiscordctl, line 32 at r3 (raw file):
Previously, bb010g (bb010g) wrote…
Oh, we only copy in on Flatpak & Snap. That changes things. I want to bring back
-cto control this?
I don't believe there's a reason to copy or link it anymore aside from Flatpak/Snap sandboxing.
The require call can just point to whichever location the file resides in as long as it's readable.
betterdiscordctl, line 298 at r3 (raw file):
Previously, bb010g (bb010g) wrote…
These warnings are (currently) one of the main ways to see which config directory actually gets chosen.
That's what verbosity is for. The warning will only show up right before an error now.
betterdiscordctl, line 377 at r3 (raw file):
Previously, bb010g (bb010g) wrote…
Double quotes on the
requirehere matches Zere's syntax, which I do care about maintaining on significant patched lines.
That's why I initially used double quotes for everything before you made it inconsistent. 🤔
There was a problem hiding this comment.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @bb010g and @ObserverOfTime)
betterdiscordctl, line 32 at r3 (raw file):
Previously, ObserverOfTime wrote…
I don't believe there's a reason to copy or link it anymore aside from Flatpak/Snap sandboxing.
Therequirecall can just point to whichever location the file resides in as long as it's readable.
If someone sticks a betterdiscord.asar in their Downloads folder, and then installs using it, there's a decent chance Downloads/betterdiscord.asar will get deleted while still in use. This shouldn't be an issue for developers, but I could see this for someone's quick & dirty trial of some arbitrary build.
betterdiscordctl, line 298 at r3 (raw file):
Previously, ObserverOfTime wrote…
That's what verbosity is for. The warning will only show up right before an error now.
It'll also show up when other config directories are tried, uselessly. This hints to someone that installing one of the warned-about Discord flavors will change betterdiscordctl's flavor selection. The idea is to get them explicitly specifying on Canary or PTB via --flavor, while also still functioning before that's specified.
betterdiscordctl, line 377 at r3 (raw file):
Previously, ObserverOfTime wrote…
That's why I initially used double quotes for everything before you made it inconsistent. 🤔
Discord's stock $core/index.js is:
module.exports = require('./core.asar');I wanted to keep our semantically identical line syntactically identical.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @bb010g and @ObserverOfTime)
betterdiscordctl, line 32 at r3 (raw file):
Previously, bb010g (bb010g) wrote…
If someone sticks a
betterdiscord.asarin their Downloads folder, and then installs using it, there's a decent chanceDownloads/betterdiscord.asarwill get deleted while still in use. This shouldn't be an issue for developers, but I could see this for someone's quick & dirty trial of some arbitrary build.
In any case, we may want to bring copy_bd back as a variable, set it on --snap & --flatpak, and switch on that internally, since it effects how we locate bd_asar_data_asar. On that note, I don't love the name bd_asar_data_asar, so what do you think of bd_asar_installed? Should be more accurate, and i can only think of either that or bd_asar_destination.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @bb010g)
betterdiscordctl, line 32 at r3 (raw file):
If someone sticks a
betterdiscord.asarin their Downloads folder, and then installs using it, there's a decent chanceDownloads/betterdiscord.asarwill get deleted while still in use. This shouldn't be an issue for developers, but I could see this for someone's quick & dirty trial of some arbitrary build.
But then we're relying on that same user also specifying --copy-bd or they will still have the same issue.
This flag is mostly meant for developers who want to test a custom build so I might just clarify that instead.
On that note, I don't love the name
bd_asar_data_asar, so what do you think ofbd_asar_installed? Should be more accurate, and i can only think of either that orbd_asar_destination.
I think bd_asar_dest should be fine.
|
Sorry, other things had came up the past couple weeks. I'll get back to this and push what should be everything necessary tomorrow morning for review. |
|
I tested this today, works perfectly! |
No, it's not. |
|
It would be great if you could merge it and updae the AUR/COPR packages as they are not working right now :/ |
|
Waiting for @bb010g to make some final changes. |
There was a problem hiding this comment.
Sorry for the long delay! Don't fall off bridges, people.
This bumps our major version to 2.0.0, as the command line interface significantly changes options. The new CLI is more robust, and shouldn't need a similar rework going forward.
Internally, code paths are much easier to follow and I tried to make it easier to figure out what variables get initialized when.
Reviewed 3 of 3 files at r17.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ObserverOfTime)
betterdiscordctl, line 32 at r3 (raw file):
Previously, bb010g (bb010g) wrote…
They'd have the same issue if composing the command on their own. On the other hand, if the alternative source gives them a
betterdiscordctlcommand to use, they're fine, which I think is reasonable in this case. I see it as "hey, I fixed that thing, download this and runbetterdiscordctl --bd-asar ~/Downloads/betterdiscord-patched.asar --copy-bdand see if your problem goes away.". Anything longer term is likely hosted at a published URL.
Handled by always copying remote data to $bd_data during remote data install if it's not already there.
|
By the way, tested on my NixOS box with a Nixpkgs ( |
There was a problem hiding this comment.
Reviewed 2 of 2 files at r14, 2 of 3 files at r17, 1 of 1 files at r18.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @ObserverOfTime)
betterdiscordctl, line 32 at r3 (raw file):
Previously, bb010g (bb010g) wrote…
Handled by always copying remote data to
$bd_dataduring remote data install if it's not already there.
I'm still against always copying it.
betterdiscordctl, line 48 at r18 (raw file):
Options: -V, --version display version info and exit
We should keep the capital letters to make it clearer where each option starts and ends.
betterdiscordctl, line 51 at r18 (raw file):
-h, --help display this help message and exit -v, --verbose increase verbosity -s, --quiet decrease verbosity
This should be -q, --quiet.
Maybe we could use verbose for all messages now that we have this?
The levels can be -2 for errors, -1 for warnings, and 0 for info.
betterdiscordctl, line 296 at r18 (raw file):
index_mod=yes elif grep -Fq "$bd_asar_name" "$d_core/index.js"; then index_mod=noncompliant
Does noncompliant mean external?
betterdiscordctl, line 305 at r18 (raw file):
github) bd_remote_status+=" BetterDiscord remote GitHub owner: $bd_remote_github_owner
Maybe combine these into a single line?
betterdiscordctl, line 314 at r18 (raw file):
;; dir) bd_remote_dir+="
This should be bd_remote_status.
betterdiscordctl, line 368 at r18 (raw file):
fi github_version=$(curl -NLSs "$upgrade_url" | sed -n 's/^VERSION=//p')
Can we change all mentions of GitHub in this function to "remote"?
Someone might be hosting a temporary fix on a pastebin or something.
README.md, line 45 at r18 (raw file):
[fuchsia-cli_help]: https://fuchsia.dev/fuchsia-src/concepts/api/cli_help TODO: Update this options section (based on `--help`?)
Maybe wrap this TODO in a comment?
README.md, line 68 at r18 (raw file):
insensitive, both hyphenated and unhyphenated). Stable is `''`, as it has no suffix. Note that **no** spaces follow colons. Your Discord flavor probably doesn't have a space in it, so don't use any in here.
Should we mention that flavors are only supported in traditional installs?
README.md, line 146 at r18 (raw file):
* `betterdiscordctl -f ptb status` Shows the BetterDiscord for the PTB flavor.
"Shows the BetterDiscord status for the PTB flavor."
shell.nix, line 1 at r18 (raw file):
{ nixpkgs ? import <nixpkgs> { } }:
What does this file do?
There was a problem hiding this comment.
Reviewed 3 of 3 files at r20.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @ObserverOfTime)
betterdiscordctl, line 32 at r3 (raw file):
Previously, ObserverOfTime wrote…
I'm still against always copying it.
This won't copy on normal remote installs. It copies into $bd_config/data/$bd_asar_name if necessary (by default $bd_config/data/betterdiscord.asar), which is also the default download target $bd_remote_dir/$bd_remote_asar (which is $bd_config/data/$bd_remote_asar, which is $bd_config/data/betterdiscord.asar).
This will probably only copy on --bd-remote-dir or --bd-remote-url. It's a safety net.
betterdiscordctl, line 51 at r18 (raw file):
Previously, ObserverOfTime wrote…
This should be
-q, --quiet.Maybe we could use
verbosefor all messages now that we have this?
The levels can be-2for errors,-1for warnings, and0for info.
Thanks for catching that; somehow this got swapped in the documentation only. I want betterdiscordctl -v or betterdiscordctl --verbose to be valid, as is standard, so a numeric betterdiscordctl --verbosity 2 for VV would work but it seems like an option we don't really need. We could add it if you think it's worthwhile.
betterdiscordctl, line 296 at r18 (raw file):
Previously, ObserverOfTime wrote…
Does
noncompliantmean external?
noncompliant means that there's an injection going on but it's not what we're expecting given the current configuration (as we're testing off of the exact betterdiscord.asar path now). I don't know a better name for it. (Originally, this was maybe, but that's crappy.)
betterdiscordctl, line 305 at r18 (raw file):
Previously, ObserverOfTime wrote…
Maybe combine these into a single line?
I thought about that, but the line can get long, and this shows separation better & is easier to copy? Separate lines are boring, easy to parse, and functional.
betterdiscordctl, line 314 at r18 (raw file):
Previously, ObserverOfTime wrote…
This should be
bd_remote_status.
Done.
betterdiscordctl, line 368 at r18 (raw file):
Previously, ObserverOfTime wrote…
Can we change all mentions of GitHub in this function to "remote"?
Someone might be hosting a temporary fix on a pastebin or something.
Went with "self-upgrade" because that's what we're already using with $self_upgrade_url & "Self-upgrade URL".
Also, made this consistently "self-upgrade" now since I forgot to change that over.
README.md, line 45 at r18 (raw file):
Previously, ObserverOfTime wrote…
Maybe wrap this TODO in a comment?
Thanks for catching this; finished the documentation. If you ever feel inclined to rewrite the documentation, feel free. :p
I still need to convert this to a manpage at some point, especially since we're packaged by a couple more places now and mdoc-to-md is just sitting around.
README.md, line 68 at r18 (raw file):
Previously, ObserverOfTime wrote…
Should we mention that flavors are only supported in traditional installs?
Done.
README.md, line 146 at r18 (raw file):
Previously, ObserverOfTime wrote…
"Shows the BetterDiscord status for the PTB flavor."
Done.
shell.nix, line 1 at r18 (raw file):
Previously, ObserverOfTime wrote…
What does this file do?
It's handy because I use Nixpkgs/Nix (on NixOS). This guarantees me shellcheck and bash whenever I change into this directory, in combination with direnv & lorri. Decided to commit it for anyone else developing with Nixpkgs.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @ObserverOfTime)
betterdiscordctl, line 48 at r18 (raw file):
https://fuchsia.dev/fuchsia-src/concepts/api/cli_help
Short description fragments (the second column in Options or Commands) begin with a lowercase letter and are not expected to be full sentences. Any text beyond the short description should be complete sentences, with a period after the fragment.
I think it's fine, given that the leftmost column is nearby, and that clearly tells when a new option starts & ends?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @ObserverOfTime)
betterdiscordctl, line 32 at r3 (raw file):
Previously, bb010g (bb010g) wrote…
This won't copy on normal remote installs. It copies into
$bd_config/data/$bd_asar_nameif necessary (by default$bd_config/data/betterdiscord.asar), which is also the default download target$bd_remote_dir/$bd_remote_asar(which is$bd_config/data/$bd_remote_asar, which is$bd_config/data/betterdiscord.asar).This will probably only copy on
--bd-remote-diror--bd-remote-url. It's a safety net.
Yes, but I don't want it to copy on --bd-remote-dir unless sandboxed.
betterdiscordctl, line 48 at r18 (raw file):
Previously, bb010g (bb010g) wrote…
https://fuchsia.dev/fuchsia-src/concepts/api/cli_help
Short description fragments (the second column in Options or Commands) begin with a lowercase letter and are not expected to be full sentences. Any text beyond the short description should be complete sentences, with a period after the fragment.
I think it's fine, given that the leftmost column is nearby, and that clearly tells when a new option starts & ends?
Given how I was confused for a moment, I don't think it's clear.
betterdiscordctl, line 51 at r18 (raw file):
Previously, bb010g (bb010g) wrote…
Thanks for catching that; somehow this got swapped in the documentation only. I want
betterdiscordctl -vorbetterdiscordctl --verboseto be valid, as is standard, so a numericbetterdiscordctl --verbosity 2forVVwould work but it seems like an option we don't really need. We could add it if you think it's worthwhile.
That's not what I mean. We currently have 2 debug levels with verbose 1 and verbose 2, and we use regular printf calls for info, warnings, and errors. Instead, we can use verbose 0 for info, verbose -1 for warnings, and verbose -2 for errors, to make it completely quiet.
betterdiscordctl, line 305 at r18 (raw file):
Previously, bb010g (bb010g) wrote…
I thought about that, but the line can get long, and this shows separation better & is easier to copy? Separate lines are boring, easy to parse, and functional.
Reusing BetterDiscord remote URL: $bd_remote_url" by combining the parts would be fine.
betterdiscordctl, line 368 at r18 (raw file):
Previously, bb010g (bb010g) wrote…
Went with "self-upgrade" because that's what we're already using with
$self_upgrade_url& "Self-upgrade URL".Also, made this consistently "self-upgrade" now since I forgot to change that over.
"Self-upgrade version" doesn't make sense to me.
betterdiscordctl, line 363 at r20 (raw file):
} bdc_self-upgrade() {
I don't like hyphens in function names.
README.md, line 45 at r18 (raw file):
Previously, bb010g (bb010g) wrote…
Thanks for catching this; finished the documentation. If you ever feel inclined to rewrite the documentation, feel free. :p
I still need to convert this to a manpage at some point, especially since we're packaged by a couple more places now and mdoc-to-md is just sitting around.
You forgot to remove the TODO though.
More heavily based on the Fuchsia guidelines this time. https://fuchsia.dev/fuchsia-src/concepts/api/cli https://fuchsia.dev/fuchsia-src/concepts/api/cli_help wheee, shell verbosity.
i need to publish this upstream. it's slightly more robust.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r21.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @ObserverOfTime)
betterdiscordctl, line 32 at r3 (raw file):
Previously, ObserverOfTime wrote…
Yes, but I don't want it to copy on
--bd-remote-dirunless sandboxed.
This keeps behavior consistent, though. What do you lose from copying? If you're using this for development, betterdiscordctl install after you rebuild the asar is basically instant.
betterdiscordctl, line 48 at r18 (raw file):
Previously, ObserverOfTime wrote…
Given how I was confused for a moment, I don't think it's clear.
Uppercase letters aren't a reliable indicator either due to how sentences can begin right after line breaks. This happens in the current --d-flavors & --d-modules descriptions (Defaults to).
betterdiscordctl, line 51 at r18 (raw file):
Previously, ObserverOfTime wrote…
That's not what I mean. We currently have 2 debug levels with
verbose 1andverbose 2, and we use regularprintfcalls for info, warnings, and errors. Instead, we can useverbose 0for info,verbose -1for warnings, andverbose -2for errors, to make it completely quiet.
I want default verbosity to be level 0. Verbosity level -1 would be more quiet than default, like with curl.
Our function verbose prints to standard error, which is not what we want for all output. We could use verbose 0 for WARN or ERROR, but we already use die for ERROR.
betterdiscordctl, line 305 at r18 (raw file):
Previously, ObserverOfTime wrote…
Reusing
BetterDiscord remote URL: $bd_remote_url"by combining the parts would be fine.
Merged into a single line with the same syntax used for input.
betterdiscordctl, line 368 at r18 (raw file):
Previously, ObserverOfTime wrote…
"Self-upgrade version" doesn't make sense to me.
We're already using remote for bd_remote though? Also, self_upgrade_url makes sense. Changed the user-facing text, though.
betterdiscordctl, line 363 at r20 (raw file):
Previously, ObserverOfTime wrote…
I don't like hyphens in function names.
It matches our command having a hyphen, and avoids ambiguity with a theoretical bdc_self set of functions. Be we can't avoid that with variable names, so sure.
README.md, line 45 at r18 (raw file):
Previously, ObserverOfTime wrote…
You forgot to remove the TODO though.
Derp.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r20, 2 of 2 files at r21.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ObserverOfTime)
betterdiscordctl, line 32 at r3 (raw file):
Previously, bb010g (bb010g) wrote…
This keeps behavior consistent, though. What do you lose from copying? If you're using this for development,
betterdiscordctl installafter you rebuild the asar is basically instant.
Several pointless I/Os. ¯\_(ツ)_/¯
betterdiscordctl, line 51 at r18 (raw file):
Previously, bb010g (bb010g) wrote…
I want default verbosity to be level 0. Verbosity level -1 would be more quiet than default, like with curl.
Our function
verboseprints to standard error, which is not what we want for all output. We could useverbose 0forWARNorERROR, but we already usedieforERROR.
Level 0 is the default and, as far as I can tell, there is no -1 now.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ObserverOfTime)
betterdiscordctl, line 32 at r3 (raw file):
Previously, ObserverOfTime wrote…
Several pointless I/Os. ¯_(ツ)_/¯
If people report friction due to this, we can change it later. I'd like to keep this consistent if possible.
betterdiscordctl, line 51 at r18 (raw file):
Previously, ObserverOfTime wrote…
Level 0 is the default and, as far as I can tell, there is no -1 now.
Correct. We haven't implemented quieter operation so far. If we do pursue that, introducing something like verbose that doesn't print to stderr would be a good idea.
updatecommand inreinstall(resolves [Feature request] Auto-update injector on (re)install #76).--scanand--discordas we don't really need them.--nixsince the modules location isn't affected.--copy-bdwhich was deemed unnecessary.bd_map_*&bd_injectorfunctions.bd_data&bd_asarfunctions.--bd-repo-branchto--bd-repo-release.--betterdiscordto specify a custombetterdiscord.asar.$XDG_DATA_HOME/betterdiscordctldirectory.This change is