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

Assume base_url is the API url, not the bare domain #12

Closed
wants to merge 1 commit into from

Conversation

0atman
Copy link

@0atman 0atman commented Apr 6, 2024

Problem

I don't know if I'm right here - but I was confused that miniflux.app has /v1/ already appended to the API Endpoint when you make an api key:

image

But writing that into ~/.config/cliflux/config.toml causes this error in cliflux:

Error 404 Not Found: HTTP status client error (404 Not Found) for url   
(https://reader.miniflux.app/v1/v1/entries?status=unread&order=published_at&direction=desc&limit=100&offset=0)                                         

note /v1/v1/ in the formatted url.

What this PR does to fix it

  • Removes the hard-coded /v1/s in the 3 places in the code it's referenced.

Bonuses

Rationale

This of course could easily be fixed by removing /v1/ in the config file when you paste in your base_url, but I think the assumption is wrong:

base_url should be the API base, not the bare domain, I think?

Thank you!

@spencerwi
Copy link
Owner

spencerwi commented Apr 8, 2024

I don't think I agree with this change for a few reasons:

  • Philosophical: users of Miniflux don't think of using Miniflux by visiting the api's URL, but by visiting the URL of their running Miniflux instance. The most obvious "what URL are they asking me for here" is "the one in the address bar when I have the Miniflux web UI open".

  • Forward-compatible: In the event that there's ever a /v2/ API for Miniflux, I want cliflux to be able to choose to use it only when we know the code is compatible, rather than having users put "whatever the highest number is" in this config field, and then being puzzled at why cliflux is broken.

  • Backward-compatible: I dunno how many people use this little personal project I wrote, but it seems like the answer is "more than just me" (which is still a pleasant surprise for me!). Implementing this change would be a breaking change, that would require existing users to read release notes to learn why their cliflux app isn't working, and then make changes to their existing config file. That's a cost to existing users, and so it should have a clear benefit. I don't think this shift is a clear benefit in that regard.

That said, if this is a clearer mental model for you, I'm not offended by your use of a personal fork.

@0atman
Copy link
Author

0atman commented Apr 25, 2024

Very fine points! Thank you again 😄

@0atman 0atman closed this Apr 25, 2024
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