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.

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

refactor: Remove RuntimeConfig in favor of config variables, Closes #106 #148

Merged
merged 1 commit into from Sep 15, 2020

Conversation

rubensworks
Copy link
Contributor

This uses the new Components.js variables functionality.

@rubensworks
Copy link
Contributor Author

Build fails at the moment because componentsjs-compile-config does not support variables yet. Will look into that next (forgot about it...).
In the meantime this PR can already be reviewed though.

Copy link
Member

@RubenVerborgh RubenVerborgh left a comment

Choose a reason for hiding this comment

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

Amazing work, and exactly what we needed!
Best to merge after #142, which touches some of this.

@@ -10,6 +10,7 @@
"files-scs:config/presets/ldp/request-parser.json",
"files-scs:config/presets/setup.json",
"files-scs:config/presets/storage.json",
"files-scs:config/presets/storage_wrapper.json"
"files-scs:config/presets/storage_wrapper.json",
"files-scs:config/presets/variables.json"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe give a more descriptive name? Here it can be runtime-settings or cli-params for me.

"@id": "urn:solid-server:default:variable:base"
},
"Setup:_port": {
"@id": "urn:solid-server:default:variable:port"
Copy link
Member

Choose a reason for hiding this comment

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

🤘

config: { type: 'string', alias: 'c' },
})
.help();

new Promise<RuntimeConfig>(async(resolve): Promise<void> => {
new Promise<string>(async(resolve, reject): Promise<void> => {
Copy link
Member

Choose a reason for hiding this comment

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

The whole promise wrap thingy is weird here. Can't we just make this entire function async?

Copy link
Member

Choose a reason for hiding this comment

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

…or if we can't, just call an async function that does it?

Copy link
Member

Choose a reason for hiding this comment

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

Link to @rubensworks reply when I asked the same thing. But yea, moving this part to a separate async function could make it cleaner.

Copy link
Member

@joachimvh joachimvh left a comment

Choose a reason for hiding this comment

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

Great ^^. But yea, will need to be rebased after #142 (and potentially #143).

@rubensworks
Copy link
Contributor Author

Resolved all comments and rebased. So this one should be good to go once Travis finishes.

@joachimvh
Copy link
Member

@rubensworks can you rebase this to master? I'll wait with the metadata PRs until this is merged to make the rebase easier 😄

@rubensworks
Copy link
Contributor Author

@joachimvh Done

@joachimvh
Copy link
Member

Another victim of the "rebase doesn't run pre-commit checks" feature

@rubensworks
Copy link
Contributor Author

That's why we have Travis :)

@joachimvh joachimvh merged commit 1dd140a into master Sep 15, 2020
@joachimvh joachimvh deleted the refactor/runtimeconfig branch September 15, 2020 06:52
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.

None yet

3 participants