Converting create_instance to 2 separate functions #6169
Conversation
Attempted build_from_crawler
Separates the create_instance into 2 separate functions 1) build_from_settings 2)build_from_crawler
Commented out function duplicates
Edited import statement and changed create_instance call to build_from_settings
Testing 2 new functions build_from_settings build_from_crawler
edited import statement and changed create_instance calls to build_from_settings
edit import statements and change create_instance calls
Create_instance changed to build_from_crawler
edit create_instance function
create_instance changed to build_from_crawler
edit import sentence and change create_instance call
|
You should use pre-commit and you should run the tests locally before submitting. |
|
Have you run the tests locally? |
|
Attempted to run the tests locally but are running into issues. Will attempt to debug and will make another commit once resolved. |
Co-authored-by: Andrey Rakhmatullin <wrar@wrar.name>
Co-authored-by: Andrey Rakhmatullin <wrar@wrar.name>
|
Hi @wRAR - thank you for the detailed feedback - is our commit merge ready or would you like us to make more changes? |
|
Hi! Sorry to bother you again, this PR is actually part of our final assignment that has significant credit, and having it merged would help us immensely. Could you check over the changes again and, if possible, merge it? Thank you so much @wRAR |
tests/test_utils_misc/__init__.py
Outdated
| if hasattr(m, "from_crawler"): | ||
| m.from_crawler.assert_called_once_with(crawler, *args, **kwargs) | ||
| self.assertEqual(m.call_count, 0) | ||
| else: |
There was a problem hiding this comment.
So this test only checks from_crawler while test_build_from_settings checks from_crawler and from_settings, I think this is an error, this one should check both while test_build_from_settings should only check from_settings.
tests/test_utils_misc/__init__.py
Outdated
| @@ -111,47 +142,23 @@ def _test_with_settings(mock, settings): | |||
| else: | |||
| mock.assert_called_once_with(*args, **kwargs) | |||
|
|
|||
| def _test_with_crawler(mock, settings, crawler): | |||
There was a problem hiding this comment.
So this logic should be kept in _test_with_crawler.
tests/test_utils_misc/__init__.py
Outdated
| # 1. with no alternative constructors | ||
| # 2. with from_settings() constructor | ||
| # 3. with from_crawler() constructor | ||
| # 4. with from_settings() and from_crawler() constructor |
tests/test_utils_misc/__init__.py
Outdated
| @@ -95,14 +96,44 @@ class TestItem(Item): | |||
| list(arg_to_iter(TestItem(name="john"))), [TestItem(name="john")] | |||
| ) | |||
|
|
|||
| def test_create_instance(self): | |||
There was a problem hiding this comment.
I also think the create_instance test should be kept as long as we keep the function itself.
|
Hi @wRAR - apologies for another ping - Was wondering if our PR can be merged with the base branch now if it's ready? Sorry for the slight rush, our deadline is in a day and we would receive credit if it is successfully merged...however, if there are still changes to be made, we will work accordingly to get those done as well. Thank you for your understanding! |
|
@sa2415 our policy is to merge contributor PRs after two approvals from maintainers so we can merge it once @Gallaecio approves it. |
|
Awesome! |
Converted
create_instanceto 2 separate functions (build_from_crawlerandbuild_from_settings).Changed other calls to
create_instancein other files.Added tests for the new functions.
Fixes #5523, closes #5884, closes #6162.