Skip to content

Expand SolidHost config constructor, plus tests - #1127

Merged
kjetilk merged 1 commit into
release/v5.0.0from
expand-server-host
Mar 15, 2019
Merged

kjetilk merged 1 commit into
release/v5.0.0from
expand-server-host

Conversation

@dmitrizagidulin

Copy link
Copy Markdown
Contributor

Minor refactor, expand the SolidHost constructor.

Extracted from PR #702, partly addresses issue #483.

@megoth
megoth requested review from RubenVerborgh and kjetilk March 5, 2019 03:55

@kjetilk kjetilk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me, but I don't quite understand the impact of just passing argv, so I think there should be more reviews.

@kjetilk
kjetilk requested a review from michielbdejong March 8, 2019 16:07
@dmitrizagidulin

Copy link
Copy Markdown
Contributor Author

@kjetilk so, the impact of the change (just passing in the argv config object directly) is quite minimal. You can think of the SolidHost instance as a class that holds (some of) the config attributes of argv.

@michielbdejong michielbdejong left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, assuming that there will then be other code that reads these new variables (host.live, host.root, host.multiuser, host.webid). Is this adding them as runtime configurable (as opposed to read from the config)? If so, then they should also be added to the usage documentation somewhere, probably?

@dmitrizagidulin

Copy link
Copy Markdown
Contributor Author

@michielbdejong Yes, there is downstream code that checks those params. (And no, they're not live-configurable, they're just from the config file only)

@kjetilk
kjetilk merged commit 21ebe47 into release/v5.0.0 Mar 15, 2019
@kjetilk
kjetilk deleted the expand-server-host branch March 15, 2019 14:45
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