Skip to content

Thread and exploration options#42

Merged
jcoupey merged 7 commits intoVROOM-Project:masterfrom
cedricroc:master
Mar 24, 2020
Merged

Thread and exploration options#42
jcoupey merged 7 commits intoVROOM-Project:masterfrom
cedricroc:master

Conversation

@cedricroc
Copy link
Copy Markdown

Override thread and explore options. Same geometry option.

@jcoupey
Copy link
Copy Markdown
Contributor

jcoupey commented Mar 20, 2020

Thanks @cedricroc for this PR! It could indeed be convenient to be able to expose more parameters on the client-side.

For consistency, I think that the config parameters in config.yml should have the same type as the command-line parameters they stand for (see vroom usage).

So threads and explore could be integers in config.yml. Then if override is true, we'd check for input values to replace the defaults (that's just a matter of simplifying the new checks in index.js).

Another benefit of this approach would be that people that don't want to expose those parameters on the client-side would now have a simple way to change the defaults right from the config.

@jcoupey jcoupey added this to the v0.6.0 milestone Mar 20, 2020
@cedricroc
Copy link
Copy Markdown
Author

Hi @jcoupey,
I update with your comment. Thread and exploration options are fixed by default value in vroom-backend.
What do you think of this version ?

Comment thread src/index.js Outdated
typeof req.body.options.t == "number"
) {
reqOptions.push('-t ' + req.body.options.t);
args.threads = req.body.options.t;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This would override the default value at server level, so subsequent requests with no t option won't use the default setting any more. We should define a local threads variable initialized with args.threads before this check and update if required.

Comment thread src/index.js Outdated
typeof req.body.options.x == "number"
) {
reqOptions.push('-x ' + req.body.options.x);
args.explore = req.body.options.x;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same as for threads.

@cedricroc
Copy link
Copy Markdown
Author

I've not seen replace default value. I corrected

@jcoupey jcoupey merged commit 650067e into VROOM-Project:master Mar 24, 2020
@jcoupey
Copy link
Copy Markdown
Contributor

jcoupey commented Mar 24, 2020

Thanks @cedricroc for the changes and submitting this PR!

I've done a couple minor adjustements:

  • in the merge commit, reorder options in src/config.js to silence es-lint warnings
  • in f19b3a0, remove the uncessary req.body.options.t and req.body.options.x checks. This was causing the check to be false when passing "x": 0 so -x 5 was still sent in the options.

jcoupey added a commit that referenced this pull request Mar 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants