-
Notifications
You must be signed in to change notification settings - Fork 360
UI: API Break changes more verbose in terminal. Colorized some commands terminal output. #2235
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
base: dev
Are you sure you want to change the base?
Conversation
Deploy on GitHub Pages
…rminal output for commands
| @@ -0,0 +1,33 @@ | |||
| name: "Deploy new pages build" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems accidental? In any case, revert - we already have one of these.
| Terminal.print("This executable cannot be run."); | ||
| Terminal.print("DeepscanV1.exe lets you run 'scan-analyze' with a depth up to 5."); | ||
| Terminal.error("This executable cannot be run."); | ||
| Terminal.warn("DeepscanV1.exe lets you run 'scan-analyze' with a depth up to 5."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't feel like a warning to me.
| Terminal.print("This executable cannot be run."); | ||
| Terminal.print("DeepscanV2.exe lets you run 'scan-analyze' with a depth up to 10."); | ||
| Terminal.error("This executable cannot be run."); | ||
| Terminal.warn("DeepscanV2.exe lets you run 'scan-analyze' with a depth up to 10."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same.
| } | ||
|
|
||
| Terminal.print(targetServer.hostname + ":"); | ||
| if (targetServer.hasAdminRights) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Admin rights are not relevant to hacking. We should leave this out, to avoid furthering this misconception.
| } | ||
| Terminal.print("Server base security level: " + targetServer.baseDifficulty); | ||
| Terminal.print("Server current security level: " + targetServer.hackDifficulty); | ||
| if (targetServer.hackDifficulty > targetServer.baseDifficulty) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is complicated and I'm not sure that it's a good idea. Let's revert this part for now, and do a different PR later that focuses on augmenting ServerProfiler if you still want to later. (The built-in.exe tools are intentionally crude to nudge you towards writing your own.)
| Terminal.print("AutoLink.exe lets you automatically connect to other servers when using 'scan-analyze'."); | ||
| Terminal.print("When using scan-analyze, click on a server's hostname to connect to it."); | ||
| Terminal.error("This executable cannot be run."); | ||
| Terminal.warn("AutoLink.exe lets you automatically connect to other servers when using 'scan-analyze'."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to comment on all of the following simply with "info" to say that these are informational messages only so I don't think it's appropriate to use .warn
| Terminal.print("This executable cannot be run."); | ||
| Terminal.print("Formulas.exe lets you use the formulas API."); | ||
| Terminal.error("This executable cannot be run."); | ||
| Terminal.warn("Formulas.exe lets you use the formulas API."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Info
| ? `\n\nWe found ${pluralize(detail.totalDetectedLines, "affected line")}.` | ||
| : ""), | ||
| ); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra newline
src/utils/APIBreaks/APIBreak.ts
Outdated
| ++popUpIndex; | ||
| } | ||
| //Made terminal more verbose | ||
| for (const detail of details) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of showing the info on the terminal, possibly instead of pop-ups. But this is (incorrectly) duplicating logic that the pop-ups use. It should run in the same loop and use the same code.
|
@catloversg I've noticed when running the dev version that the number of pop-ups can be quite busy. What do you think about having the main overview still as a single pop-up, but printing the details to the console instead? (This PR currently does it as well.) This would avoid pop-up exhaustion, and also the console has scrollback. |
src/Terminal/HelpText.ts
Outdated
| " vim [files...] Text editor - Open up and edit one or more scripts or text files in vim mode", | ||
| " weaken Reduce the security of the current machine", | ||
| " wget [url] [target file] Retrieves code/text from a web server", | ||
| colors.green + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to require a systemically different approach to avoid destroying the formatting we have here.
The first step is to use template string literals. This means it's probably time to turns this whole thing into a single template string, which can embed newlines, rather than having an array of strings.
The next step is to choose a set of short, equal-sized constants to use. You only need three, for green, yellow and reset. I would suggest CMD, ARG and TXT. You can then embed those in the string via {CMD} etc, and the even spacing will be preserved.
That sounds like a good idea. |
I get it would be an only popup with all the relevant info together with a terminal output more detailed |
| cyan: "\x1b[36m", | ||
| white: "\x1b[37m", | ||
| reset: "\x1b[0m", | ||
| cmd: "\x1b[32m", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My suggestion here was to use all-caps to signify that they're constants, and then just define three separate constants CMD ARGS and TXT, not members of an object.
Normally what you have here would be the better practice, but in this specific case where they're being used so much in string formatting, keeping them small and avoiding visual noise is more important.
| "Retrieves code/text from a web server", | ||
| " ", | ||
| ]; | ||
| /* const commands = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove
| export const HelpTexts: Record<string, string[]> = { | ||
| alias: [ | ||
| colors.yellow + 'Usage: alias [-g] [name="value"] ', | ||
| 'Usage: alias [-g] [name="value"] ', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want, you can also convert these to template strings and add coloring to them. But it is by no means required; this PR is already plenty big.
| for (const detail of details) { | ||
| if (detail.totalDetectedLines > 0) { | ||
| Terminal.warn("_____________"); | ||
| for (const detail of details) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this, and thinking through the consequences, I realize that switching to writing the details in the terminal will actually be rather involved, with only the smallest part of it being showing the details in color. (There's stuff like renaming fields where "showPopup" isn't correct anymore.)
So, I'm going to suggest that we cut that from this PR to avoid score creep, and do it in a different PR (possibly in parallel) that can focus solely on that.


Uh oh!
There was an error while loading. Please reload this page.