Skip to content

Catalist: Patch Exception Handling#1818

Open
IanRFerguson wants to merge 2 commits intomainfrom
ian/patch/catalist-zip-operation
Open

Catalist: Patch Exception Handling#1818
IanRFerguson wants to merge 2 commits intomainfrom
ian/patch/catalist-zip-operation

Conversation

@IanRFerguson
Copy link
Copy Markdown
Contributor

What is this change?

Adds an exception for clarity in the CatalistMatch class if there is a failure to save the zipped file from the vendor's SFTP server. The current active exception is raising on zf.extractall(path=temp_dir), which tells us there's a problem with the file but not much else.

Breaking changes (if needed)

This is not a breaking change.

Considerations for discussion

N/A

How to test the changes (if needed)

  • (How should a reviewer test this functionality.)

Labels

Please add exactly one of the following labels to this PR:

  • breaking-change — This PR introduces a breaking change to the public API
  • non-breaking-change — This PR does not break any existing functionality

The label check CI job will fail until one of these labels is applied.

Comment thread parsons/catalist/catalist.py Outdated
@bmos
Copy link
Copy Markdown
Collaborator

bmos commented Apr 8, 2026

In case it helps, here are rebuilt tests that actually use real files.
ian/patch/catalist-zip-operation...bmos:parsons:catalist-testing

@IanRFerguson IanRFerguson added the non-breaking-change Status - Indicates that the code in this PR does not have any breaking changes. label Apr 8, 2026
@IanRFerguson
Copy link
Copy Markdown
Contributor Author

In case it helps, here are rebuilt tests that actually use real files. ian/patch/catalist-zip-operation...bmos:parsons:catalist-testing

awesome, appreciate you Ramona! looks like everything besides the label check was passing (and I've since fixed that)

should I merge your branch into this one? or what's the standard operating procedure?

@bmos
Copy link
Copy Markdown
Collaborator

bmos commented Apr 8, 2026

Merging it before merging this one is probably better in theory because it shows that the tests pass with the existing code, but I'd suggest you just merge it into your branch for simplicity because of the limited scope of this PR and knowing that I did run those tests without the changes in this PR. I'm fine with either and will be doing PR reviews with @shaunagm this afternoon if it's preferred to handle them separately.

Copy link
Copy Markdown
Collaborator

@shaunagm shaunagm left a comment

Choose a reason for hiding this comment

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

approved and happy to merge once the tests pass (I assume they're not failing from this and the PR just needs to be updated)

@bmos
Copy link
Copy Markdown
Collaborator

bmos commented Apr 15, 2026

They won't pass, the test suite needs to be changed.
There's a PR for @IanRFerguson to merge into this.
I'm going to post the PR against main since the test improvements are technically separate.

@shaunagm
Copy link
Copy Markdown
Collaborator

oh gotcha. i'll wait then!

@bmos bmos mentioned this pull request Apr 15, 2026
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

non-breaking-change Status - Indicates that the code in this PR does not have any breaking changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants