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

fetching more version based on the amount of pages we can fetch #5

Merged
merged 11 commits into from
Apr 7, 2022

Conversation

sagilaufer1992
Copy link

@sagilaufer1992 sagilaufer1992 commented Apr 6, 2022

the original implementation fetched only the first page. it means newer versions push older ones from the list of results. Therefor this fork TGENV needed a hotfix for older versions. the solution wan’t as pretty as it should have been - we used magic number for the number of pages.
As a solution added functionality for fetching the amount of pages and then fetching those pages too

@sagilaufer1992 sagilaufer1992 requested a review from a team April 6, 2022 16:27
Copy link
Member

@eranelbaz eranelbaz left a comment

Choose a reason for hiding this comment

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

Please update the description,
I don't understand the issue you are trying to solve here...

which original implementation?
since you also set default to 4


# the response header (--head) contains a link to the last page, contains the last page number and the "last" keyword to help identify
page_curl=$(curl -s --tlsv1.2 --head "$link_release")
only_relevant_part=$(echo "$page_curl" | grep -o -E "&page=[0-9]+>; rel=\"last")
Copy link
Member

Choose a reason for hiding this comment

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

only_relevant_part relevant to who?
please find better name

Copy link
Author

Choose a reason for hiding this comment

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

@sagilaufer1992
Copy link
Author

default is 4 because as for these lines were written that's how many pages there are. In case for any reason the format of the response header changes, I wished to still retrieve extra pages, not just the first, so 4 seems like a reasonable default @eranelbaz

@sagilaufer1992 sagilaufer1992 requested review from eranelbaz and a team April 6, 2022 17:16
@sagilaufer1992 sagilaufer1992 self-assigned this Apr 6, 2022
@sagilaufer1992 sagilaufer1992 merged commit 665f83f into master Apr 7, 2022
@sagilaufer1992 sagilaufer1992 deleted the chore-pages-not-magic-number branch April 7, 2022 08:00
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