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

bin concept of custom finders #8214

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

chris48s
Copy link
Member

Comment on lines 119 to 121
# this feature doesn't exist any more, but the key is
# still in the API response for legacy compatibility
ret["custom_finder"] = None
Copy link
Member Author

Choose a reason for hiding this comment

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

For the sake of not making a breaking API change, I am going to keep the key in the object but it is just always null in 100% of cases.

Copy link
Member

Choose a reason for hiding this comment

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

Don't forget that this is basically not a supported API. If the devs.DC code doesn't consume this, then there's nothing to break. If it does, then I suggest we come back and remove this when that code doesn't rely on it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I think I want to get rid of any code that might be expecting this value to exist in devs.DC first then come back and remove it. In some ways, I may have approach this backwards.

@chris48s
Copy link
Member Author

Properly removing this has knock-on effects in a bunch of places that have code to consume the value which we can also remove.

Again for the sake of not making a breaking change, I think I want to leave the key in the object on devs.DC but I do want to remove stuff like https://developers.democracyclub.org.uk/api/v1/#notes-and-tips-2 and just say "this field used to be a thing, but now it is always null" or whatever.

@coveralls
Copy link

coveralls commented Jan 23, 2025

Coverage Status

coverage: 71.774% (-0.04%) from 71.811%
when pulling a685f39 on bin-custom-finder20250123
into 00213d7 on master.

@symroe
Copy link
Member

symroe commented Jan 27, 2025

I've not checked everywhere in the code here, but this looks fine.

If we're only keeping the field in the API for public reasons, then I'd rather we do that in the devs.DC code rather than here.

There are general problems with shoving an object from this service into devs.DC and then re-broadcasting it, so let's not make things harder for ourselves by supporting legacy properties across them both

@@ -11,7 +11,6 @@
},
"type": "Feature"
},
"custom_finder": null,
Copy link
Member Author

Choose a reason for hiding this comment

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

Probably not one to deal with in this PR, but I wonder if it is actually useful to keep maintaining sandbox responses on this repo now it is private?

@chris48s
Copy link
Member Author

It should be safe to completely remove the key now that devs.DC does not rely on it

@chris48s chris48s force-pushed the bin-custom-finder20250123 branch from a685f39 to e44bff6 Compare February 13, 2025 13:52
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.

3 participants