Skip to content

first pass for AP-616#4

Merged
jason-raitz merged 9 commits intomainfrom
AP-616_add_search-to-file_method
Mar 25, 2026
Merged

first pass for AP-616#4
jason-raitz merged 9 commits intomainfrom
AP-616_add_search-to-file_method

Conversation

@jason-raitz
Copy link
Contributor

@jason-raitz jason-raitz commented Mar 18, 2026

  • removed unused client.search()
  • added client.write_search_results_to_file()
  • added method to iterate through search xml results
  • added some xml fixtures for a first and last result for a sample search as well as the expected output xml for said search.

@jason-raitz
first pass for AP-616
522114a 
 - removed unused client.search()
 - added client.write_search_results_to_file()
 - added method to iterate through search xml results
 - added some xml fixtures for a first and last result for a sample
   search as well as the expected output xml for said search.
@jason-raitz jason-raitz self-assigned this Mar 18, 2026
@jason-raitz
remove search tests
1cb5879 
 - commenting out for now to use as template for new tests
@awilfox
Copy link
Member

awilfox commented Mar 18, 2026

Since the eventual goal was to migrate Willa to this as well, I don't think search should be removed.

anarchivist
anarchivist reviewed Mar 20, 2026
View reviewed changes
Copy link
Member

@anarchivist anarchivist left a comment

Choose a reason for hiding this comment

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

generally looks good. what else is left other than the tests?

@jason-raitz
added tests
1919ad6 
 - and some sensible guard statements
 - checked for some edge cases
 - lengthened max-line-length defaults to be a little friendlier
@jason-raitz jason-raitz marked this pull request as ready for review March 23, 2026 20:56
@jason-raitz
Copy link
Contributor Author

jason-raitz commented Mar 23, 2026

Questions

  • Do we want a standard max-line-length for python?
  • Do we want to formalize which python tools to use across our various python projects?
    (pylint, flake8, mypy, pydoc, uv, linting standards)
  • For client.write_search_results_to_file(), do we want to return '0' or an error when a Tind response is
    successful, but has no matches? Currently it writes nothing to a file and returns '0'.

awilfox
awilfox reviewed Mar 23, 2026
View reviewed changes
Copy link
Member

@awilfox awilfox left a comment

Choose a reason for hiding this comment

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

Looking really good, but we should find answers to your questions before merging IMO.

@awilfox
Copy link
Member

awilfox commented Mar 23, 2026 •
edited
Loading

Questions

  • Do we want a standard max-line-length for python?

There's a lot of debate in the Python community over this. PEP 8 says 79 characters. I don't think modern Python can be effectively written with a 79 character limit. PyCharm and the Google style guide use 120. I think this is too long, and indeed, with my present eyesight my font size is too large to allow 120 characters to fit on the laptop screen. I think 100 is a fair compromise and is what we used in Willa.

  • Do we want to formalize which python tools to use across our various python projects?
    (pylint, flake8, mypy, pydoc, uv, linting standards)

This would be a great discussion to have at sprint planning.

  • For client.write_search_results_to_file(), do we want to return '0' or an error when a Tind response is
    successful, but has no matches? Currently it writes nothing to a file and returns '0'.

I'm firmly on the side of 0 results not being an error or an exceptional condition.

anarchivist
anarchivist reviewed Mar 23, 2026
View reviewed changes
jason-raitz and others added 3 commits March 24, 2026 11:31
@jason-raitz @awilfox
Update tind_client/client.py
ac28514 
Co-authored-by: Anna Wilcox <AWilcox@Wilcox-Tech.com>
@jason-raitz
fixes from PR
5f7ea0f 
 - reverts max-line-length preference to 100
 - fixes some tests
 - clarifies a couple of edge cases
@jason-raitz
removed superfluous test
134811c
awilfox
awilfox approved these changes Mar 25, 2026
View reviewed changes
Copy link
Member

@awilfox awilfox left a comment

Choose a reason for hiding this comment

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

r+! Thanks for resolving everything, looks ready to me :shipit:

anarchivist
anarchivist requested changes Mar 25, 2026
View reviewed changes
Copy link
Member

@anarchivist anarchivist left a comment

Choose a reason for hiding this comment

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

rw+c (this last change was missing).

jason-raitz and others added 2 commits March 25, 2026 13:49
@jason-raitz @anarchivist
Update tind_client/client.py
3d3b2ce 
Co-authored-by: maría a. matienzo <73732+anarchivist@users.noreply.github.com>
@jason-raitz
more cleanup
d600940
@jason-raitz jason-raitz requested a review from anarchivist March 25, 2026 18:01
@jason-raitz jason-raitz merged commit d90f93b into main Mar 25, 2026
6 checks passed
@jason-raitz jason-raitz deleted the AP-616_add_search-to-file_method branch March 25, 2026 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anarchivist anarchivist anarchivist approved these changes

@awilfox awilfox awilfox approved these changes

@danschmidt5189 danschmidt5189 Awaiting requested review from danschmidt5189

@yzhoubk yzhoubk Awaiting requested review from yzhoubk

Assignees

@jason-raitz jason-raitz

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jason-raitz @awilfox @anarchivist