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

Make max page size per pull configurable #89

Merged
merged 1 commit into from
Jun 19, 2022
Merged

Make max page size per pull configurable #89

merged 1 commit into from
Jun 19, 2022

Conversation

ramSeraph
Copy link
Contributor

Had to edit it manually the couple of times I used the tool. I think this should be configurable.

@ramSeraph
Copy link
Contributor Author

related to #84

@iandees iandees merged commit 4465dcf into openaddresses:master Jun 19, 2022
@iandees
Copy link
Member

iandees commented Jun 19, 2022

Thanks!

@ramSeraph
Copy link
Contributor Author

Looks like there are test failures.. I will check.

@ramSeraph
Copy link
Contributor Author

Adding this diff fixed it

diff --git a/tests/test_cli.py b/tests/test_cli.py
index 24ca14e..1ccf93b 100644
--- a/tests/test_cli.py
+++ b/tests/test_cli.py
@@ -57,6 +57,7 @@ class TestEsriDumpCommandlineMain(unittest.TestCase):
         self.parse_return = mock.MagicMock()
         self.parse_return.url = 'http://example.com'
         self.parse_return.outfile = self.mock_outfile
+        self.parse_return.max_page_size = 1000
         self.parse_return.loglevel = logging.ERROR
         self.parse_return.jsonlines = False
         self.parse_return.fields = None

Do I need to make a new pull request? I am kind of new to the PR workflow

@iandees
Copy link
Member

iandees commented Jun 19, 2022

Sure, if you want to file another pull request that would be helpful. I should have caught it earlier - it looks like the tests didn't run on your pull request.

@ramSeraph
Copy link
Contributor Author

Created new PR at #90

I should have run the tests beforehand too :(

iandees added a commit that referenced this pull request Jun 19, 2022
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.

2 participants