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.

Sign up for GitHub

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

Jump to bottom

Add command-line processing to update settings #2025

Merged
merged 24 commits into from
Apr 14, 2022
Merged

Add command-line processing to update settings #2025

merged 24 commits into from
Apr 14, 2022

Conversation

ericpromislow
Copy link
Contributor

No description provided.

@ericpromislow ericpromislow changed the title 1222 command line args Add command-line processing to update settings Apr 12, 2022
mook-as
mook-as requested changes Apr 12, 2022
View reviewed changes
Copy link
Contributor

@mook-as mook-as left a comment

Choose a reason for hiding this comment

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

The functions need a lot more documentation before I can review it and have confidence that our understandings of what they're supposed to do match.

@ericpromislow
Copy link
Contributor Author

Forgot to mention that docs for accessing command-line args are at: #1222 (comment)

@ericpromislow ericpromislow force-pushed the 1222-command-line-args branch 2 times, most recently from 9e49fc0 to 9153e75 Compare April 13, 2022 00:06
@ericpromislow ericpromislow requested a review from mook-as April 13, 2022 20:14
@ericpromislow ericpromislow force-pushed the 1222-command-line-args branch from 291684d to 92470e6 Compare April 13, 2022 20:15
mook-as
mook-as requested changes Apr 13, 2022
View reviewed changes
Copy link
Contributor

@mook-as mook-as left a comment

Choose a reason for hiding this comment

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

The tests don't seem to be doing the right thing — the cfg is updated in-place, so it's just asserting that an object is the same as itself (rather than that it was the same as the previous value).

@ericpromislow ericpromislow requested a review from mook-as April 14, 2022 00:48
@ericpromislow
Update prefs based on command-line arguments.
b0b0498 
Signed-off-by: Eric Promislow <[email protected]>
@ericpromislow
Hardwire slicing the process.argv array to find user-supplied arguments.
5e5a1f4 
This needs to be tested on each platform in the following configurations:
- running a built app (default)
- running `npm run dev -- args...`
- running e2e tests

Signed-off-by: Eric Promislow <[email protected]>
@ericpromislow
Pull the code to find command-line arguments into a separate module.
bf69f84 
Looks like a cheat to determine if we're running in dev mode, but it's the
only possibility that has .../dev.mjs at arg 4.

Signed-off-by: Eric Promislow <[email protected]>
@ericpromislow
Whitespace fix.
bd7e86c 
Signed-off-by: Eric Promislow <[email protected]>
@ericpromislow
Add an explanation of what getUpdatableNode does.
c09acb2 
Add more typing, and reduce the code by returning null
on any failure.

Signed-off-by: Eric Promislow <[email protected]>
@ericpromislow
Clarify what the updatable-node function is doing.
ffe0565 
Signed-off-by: Eric Promislow <[email protected]>
@ericpromislow
Remove the intermediate updateFromCommandLine function with mocks
c9fc40c 
- mock `fs.writeFileSync` so that `updateFromCommandLine` can call
  `settings.save()` and unit tests don't do crazy things to the pref set.

Signed-off-by: Eric Promislow <[email protected]>
@ericpromislow
Remove useless comment.
83e2023 
Signed-off-by: Eric Promislow <[email protected]>
@ericpromislow
Remove unused variable.
5eb507d 
Signed-off-by: Eric Promislow <[email protected]>
@ericpromislow
Need to deep-copy the original prefs object.
235275c 
Otherwise we're just comparing an object with itself.

Signed-off-by: Eric Promislow <[email protected]>
@ericpromislow
Tighten up the way we determine how the app was launched.
9e86e84 
Signed-off-by: Eric Promislow <[email protected]>
@ericpromislow
Make commandLineArgs a local constant.
c7f41e7 
Signed-off-by: Eric Promislow <[email protected]>
@ericpromislow
Use more intentional methods for determining how the app was launched.
76907d3 
Signed-off-by: Eric Promislow <[email protected]>
@ericpromislow
Elaborate more in the comments.
dfc1fc7 
Signed-off-by: Eric Promislow <[email protected]>
@ericpromislow
Minor changes involving types, parens, and comments.
c7a2248 
Signed-off-by: Eric Promislow <[email protected]>
@ericpromislow
Prefer a simple loop to a reduce statement.
feb52a0 
Signed-off-by: Eric Promislow <[email protected]>
@ericpromislow
More style and type-related changes.
bf3ae9e 
Signed-off-by: Eric Promislow <[email protected]>
@ericpromislow
Prefer toThrow to toThrowError
34b66ae 
Signed-off-by: Eric Promislow <[email protected]>
@ericpromislow
Prefer toEqual to toMatchObject
e50e99c 
Signed-off-by: Eric Promislow <[email protected]>
@ericpromislow ericpromislow force-pushed the 1222-command-line-args branch from ecbeab9 to 9d5fcd4 Compare April 14, 2022 17:49
mook-as
mook-as requested changes Apr 14, 2022
View reviewed changes
@ericpromislow ericpromislow requested a review from mook-as April 14, 2022 20:28
@mook-as mook-as merged commit 630f8fc into main Apr 14, 2022
@mook-as mook-as deleted the 1222-command-line-args branch April 14, 2022 21:50
@ericpromislow ericpromislow mentioned this pull request Apr 19, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mook-as mook-as mook-as approved these changes

@adamkpickering adamkpickering Awaiting requested review from adamkpickering

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ericpromislow @mook-as