Skip to content

refactor: remove get_apps() and usage of undocumented applications API #415

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

Merged
merged 4 commits into from
May 19, 2025

Conversation

nealrichardson
Copy link
Collaborator

Intent

Remove the last caller of the undocumented GET /__api__/applications/ from this library.

Approach

get_apps() was the only caller of this API, and it was only being used in two places, and only to check whether a content item with a given name already exists. The GET /__api__/v1/content/ API accepts a name query parameter, so we could swap those to use that.

I also deleted the dashboard_url_chr() function, which oddly was exported, because content items include dashboard_url in them.

Checklist

  • Does this change update NEWS.md (referencing the connected issue if necessary)?
  • Does this change need documentation? Have you run devtools::document()?

@nealrichardson nealrichardson requested a review from toph-allen May 16, 2025 17:49
Copy link
Collaborator

@toph-allen toph-allen left a comment

Choose a reason for hiding this comment

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

Thank you @nealrichardson! Love to see code getting deleted, especially when it calls undocumented APIs.

It feels to me like get_apps() was a relic of an earlier time in connectapi, before get_content(). Perhaps left in as a way to maintain backward compatibility? Makes me wonder about other old or redundant code that's lying around.

Either way, looks good to me, and I see that you got CI working! Thank you!

@nealrichardson
Copy link
Collaborator Author

Either way, looks good to me, and I see that you got CI working! Thank you!

Yes, it looks like in older versions of Connect, if you specified both name and include as query params, the v1 content listing API would 500. We don't need the includes though for this request so it's all good.

@toph-allen
Copy link
Collaborator

Yes, it looks like in older versions of Connect, if you specified both name and include as query params, the v1 content listing API would 500. We don't need the includes though for this request so it's all good.

Huh, that's… good to know! Particularly as I had been thinking about turning on includes for the content_item() function (because it's on by default for get_content()).

@nealrichardson
Copy link
Collaborator Author

It might be fine! Only thing to do is try it :D

@nealrichardson nealrichardson merged commit fee6d72 into main May 19, 2025
19 checks passed
@nealrichardson nealrichardson deleted the rm-apps branch May 19, 2025 15:56
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