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

changed threads default #197

Closed
wants to merge 2 commits into from
Closed

changed threads default #197

wants to merge 2 commits into from

Conversation

mjurbanski-reef
Copy link

No description provided.

jsuchan-reef
jsuchan-reef reviewed Aug 16, 2023
View reviewed changes
CHANGELOG.md Outdated
@@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Changed
* Better help text for --corsRules
* if `--threads` is not explicitly set number of threads is no longer guaranteed to be 10
Copy link

Choose a reason for hiding this comment

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

btw where/how is this guarantee violated? The default value is still 10, no?

Suggested change
* if `--threads` is not explicitly set number of threads is no longer guaranteed to be 10
* if `--threads` is not explicitly set, number of threads is no longer guaranteed to be 10

Copy link
Author

Choose a reason for hiding this comment

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

the default is 10, but the idea is that in future it maybe very well automatically adjusted and we don't want users relying on the fact it is 10 or, in fact, any specific number.

Copy link

Choose a reason for hiding this comment

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

then I'd assume this note to appear in the changelog when such change will be in effect.

@mjurbanski-reef mjurbanski-reef force-pushed the new_threads_default branch 2 times, most recently from 6720a29 to d54533f Compare August 16, 2023 15:10
@agoncharov-reef
Copy link

Just saw this accidentally. Since you are modifying download_manager, maybe you would find it handy to update this test: https://github.com/reef-technologies/B2_Command_Line_Tool/blob/master/test/unit/test_console_tool.py#L2364

@mjurbanski-reef
Copy link
Author

Just saw this accidentally. Since you are modifying download_manager, maybe you would find it handy to update this test: https://github.com/reef-technologies/B2_Command_Line_Tool/blob/master/test/unit/test_console_tool.py#L2364

the call to download_manager stays exactly the same as it was; I'm unsure what change you expected to be changed in that test

@mjurbanski-reef
Copy link
Author

merged upstream couple months ago

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsuchan-reef jsuchan-reef jsuchan-reef approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@mjurbanski-reef @agoncharov-reef @jsuchan-reef