Skip to content

Towards using only next endpoint for the major data. (Partial solution to #5003) #5237

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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

mk-pmb
Copy link

@mk-pmb mk-pmb commented Apr 11, 2025

From my discussion with @unixfox , these are the changes that I was able to salvage from #5003 that either were not critizised in code review or where I was able to apply the suggestions.

@absidue
Copy link
Contributor

absidue commented Apr 11, 2025

Looks like some of the commits in this branch are outdated as it is missing changes that are already in the master branch such as #4934.

@mk-pmb
Copy link
Author

mk-pmb commented Apr 11, 2025

That may well be. I don't understand enough crystal to be able to identify redundant code; I could only transplant changes that @unixfox made. If you tell me which (parts of) commits I shall omit, I can of course do that.

@mk-pmb
Copy link
Author

mk-pmb commented Apr 11, 2025

There's also this commit that looks important: mk-pmb@108ee36
… but should probably be solved by instead duplicating the unconditional part at the end into the tiny "else" branch and adding early return (then invert the condition to make it a tiny "then" branch), to avoid having to indent the huge original "then" branch.

(If you do decide to indent, please do that in a separate commit or git will make a reeeeeeally confusing diff. It will be bad enough already but at least that way a diff with proper ignore-whitespace options can assure you that no actual code was changed.)

Shouldn't really belong to this PR though.

Update: I learned enough crystal to do the early return transformation, but now I see that the common tail below has grown with companion stuff, so probably now the better choice is to factor out the "then" branch into its own function.

@mk-pmb mk-pmb force-pushed the use-only-next-for-extra-info-250411 branch from 3b761f4 to a9dfc70 Compare April 11, 2025 17:04
@mk-pmb
Copy link
Author

mk-pmb commented Apr 11, 2025

Ok yeah, some things seem to have been broken by git's rebase. I now discovered the UTC fallback, and it seems obvious even to me that the rebased commit does the opposite of what it claims. I removed that commit.

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