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

Add `tag-changelog-link` feature #1998

Merged

Conversation

Projects
None yet
3 participants
@HardikModha
Copy link
Contributor

commented Apr 29, 2019

Should fix #966.

I tried to use the API but I couldn't find a way to fetch details of next and previous tags and fetching all tags at once wasn't feasible. (See discussion #966)

I've taken the following approach.

  1. First, I've extracted all tags, commit ids of those tags.

  2. Now, iterating over each tag, I see if the next tag has the different commit id. (Given the fact that we can have multiple tags on the same commit id).

  3. If the next tag has different commit id, then I further check whether the tags are namespaced or not. If the namespace is the same then pick that tag for comparison.

  4. If the next tag has same commit id then that tag is skipped and we check remaining tags.

  5. For the last tag, I fetch the next page using fetchDom and perform the same steps as mentioned above. If a tag to compare is not found on the next page then we skip the tag.

Tested it on the following (and many other) repos.

  1. https://github.com/parcel-bundler/parcel/releases (It has multiple, namespaced and normal semver style tags)
  2. https://github.com/tensorflow/tensorflow/releases (With Alpha, Beta kind of tags)
  3. https://github.com/facebook/react/releases

The feature works on the following pages:

  1. releases listing: e.g. https://github.com/parcel-bundler/parcel/releases
  2. tags listing: e.g. https://github.com/parcel-bundler/parcel/tags
  3. Single tag pages: https://github.com/parcel-bundler/parcel/releases/tag/v1.10.3

Limitations:

  1. Namespaced tags are only checked within the single page to find it's previous tag. Update: Now we check on the next page, too.

  2. For the last tag on the page, we only check on the next page to find the previous tag. Update: For all the tags on any page, we also check the next page for finding its previous tag.

  3. The tags are not semantically compared. But we can easily implement it.

PS: Please suggest a better way (if you have) to solve this issue. I would be more than happy to implement it. :)

HardikModha added some commits Apr 28, 2019

Basic version of what-changes-since-previous-release ready
1. Edge cases not yet handled.
2. Currently only fetches the next release from the sequence and adds the compare link.
3. For the last tag on any page, uses fetchDom to retrieve the next page and extracts
the tag from it.
Handling different tags on same commit in a single page
Now, If there are multiple tags on the same commit, we traverse
all the commits present on the single page and select the next
commit on which different tag is present.

Still not handling the case of namespaced tags.
Considering the namespaced tags
The compare link is generated as follows

1. We find a commit having diffrent commit hash.

2. Once such a commit is found, we check the namespace.
If namespaces are same, we use that tag as the previous tag else
we try to look further for tag of same namespace.

3. If tag having same namespace is not found on the current page, we
use the next commit having different hash.

4. For the last tag on the page, we fetch the next page and follow the
same process as above.
@HardikModha

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

@bfred-it @sindresorhus Can you please take a look? Thanks!

Show resolved Hide resolved source/content.ts Outdated
@bfred-it
Copy link
Collaborator

left a comment

The code is lengthy, consider reducing repetition where possible and skip conditions.

For example:

  • getCommitIdSelector and getTagSelector can be skipped if you just use commitAnchorSelector and tagAnchorSelector respectively? This lets you skip a lot of code and even isReleasesListPage and isTagsListPage themselves
  • the code after allTagsAnchor and nextPageAllTagsAnchor is nearly identical and can probably be merged.

Generally, instead of creating multiple large code paths (if (){20 lines of code} else {20 lines of code}), try to make standardize the input content. e.g.

for each link on the page (whether it’s one or 20)
	compareLink = findNextLink() || findNextLink(memoizedFetchNextPage())
	add compareLink next to title
end for

findNextLink() {
	if tag has prefix
		return query(href ^= prefix)
	end if
	
	return query(:not(href ^= prefix))
}

Also always use for-of and not a regular for

I'm confident this code can be at least halved :)

I think this article improved my code: https://medium.com/@bartobri/applying-the-linus-tarvolds-good-taste-coding-requirement-99749f37684a


Position: I was expecting this link to be on the commit | zip | tar.gz line and something more clear than ... (I didn't even see the link at first, that ... is generally used to expand the content)

HardikModha added some commits Apr 30, 2019

Removed extra code
1. Simplified the regex
2. Simplified the tag and commit selectors and removed their unnecessary methods
3. Used the GitHub's diff icon for displaying comparison link
4. Better position for compare link
Further simplify the code
1. Removed conditional checks before looking for previous tags. All
tags are now treated equally whether they are on releases, tag listing
or single tag page.
2. Use the single loop to go through the tags irrespective of their index.
It also improved the probability of finding the previous tag to compare
as we now look into the next page for all the tags.
@HardikModha

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

  • getCommitIdSelector and getTagSelector can be skipped if you just use commitAnchorSelector and tagAnchorSelector respectively? This lets you skip a lot of code and even isReleasesListPage and isTagsListPage themselves

Done. Simplified the commit id and tag selectors. Also, removed the checks for isReleaseListPage and isTagsListPage by using the common classname as selector which was present on both the pages.

  • the code after allTagsAnchor and nextPageAllTagsAnchor is nearly identical and can probably be merged.

Now, Appending the next page tags into the same array and iterating only once. Check for the last tag is also removed.

Also always use for-of and not a regular for

Here, I need an index of the item so that I can start looking for the tags to match just after that index. You can take a look from Line no 36 and Line no 62.

Position: I was expecting this link to be on the commit | zip | tar.gz line and something more clear than ... (I didn't even see the link at first, that ... is generally used to expand the content)

Initially, I had chosen the same. The same line as commit | zip | tar.gz and for the releases just below the commit hash. (As displayed in the screenshot)

1ec2a437-cc8e-45eb-97f7-4d15ac433700

But there was some miscommunication due to #966 (comment) and I put the link on ... 😓.

HardikModha added some commits Apr 30, 2019

@HardikModha

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

@bfred-it Please take a look at new changes and suggest me if anything still needs to be changed. :)

@HardikModha

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2019

Using your suggested approach as for edge case (5c0d57f) my approach silently failed but your approach threw an error which I really liked which led me to realize what mistake I was making. Also in your suggested approach, we only need to iterate only once to extract tag and commit details. (Removed the big comment which now I think is unnecessary.)

Let me know if any more changes are needed. Thanks :)

@bfred-it
Copy link
Collaborator

left a comment

Thank you and sorry for all the reviews ❤️

Show resolved Hide resolved source/features/tag-changelog-link.tsx Outdated
Show resolved Hide resolved source/features/tag-changelog-link.tsx Outdated
Show resolved Hide resolved source/features/tag-changelog-link.tsx Outdated
Show resolved Hide resolved source/features/tag-changelog-link.tsx Outdated

HardikModha added some commits May 5, 2019

@HardikModha

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2019

Thank you and sorry for all the reviews

Haha...No need for sorry. I learned a lot through your and Sindre's hardcore reviews. Most of the time I (or any new person) is focused on making something to work and we forget some scenarios or make some silly mistakes. That's where you guys come into the picture. Just making something to work is not enough. I really appreciate your and Sindre's attitude towards perfectness. :)

@bfred-it
Copy link
Collaborator

left a comment

Also, perhaps this should use tiny-version-compare to ensure that the tag we're comparing to is actually lower than the current one.

For example on: https://github.com/tensorflow/tensorflow/releases
The tag right after v1.12.1 is TensorFlow 2.0.0-alpha0 and the generated link shouldn't be:

tensorflow/tensorflow@v2.0.0-alpha0...v1.12.1

but it should compare it against v1.12.0, further down the page

tensorflow/tensorflow@v1.12.0...v1.12.1

Show resolved Hide resolved source/features/tag-changelog-link.tsx Outdated
Show resolved Hide resolved source/features/tag-changelog-link.css Outdated
@HardikModha

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

Also, perhaps this should use tiny-version-compare to ensure that the tag we're comparing to is actually lower than the current one.

For example on: https://github.com/tensorflow/tensorflow/releases
The tag right after v1.12.1 is TensorFlow 2.0.0-alpha0 and the generated link shouldn't be:

tensorflow/tensorflow@v2.0.0-alpha0...v1.12.1

but it should compare it against v1.12.0, further down the page

tensorflow/tensorflow@v1.12.0...v1.12.1

Yep, you are right. I am aware of this limitation and have already mentioned it in a description. I didn't look for any packages (pure out of laziness maybe) 😅. Thanks for the tiny-version-compare suggestion. This wekend, I'm out so won't be able to work on it. Next week I'll give it a try.

bfred-it added some commits May 10, 2019

Shorten selectors and update+merge comments
`.release-main-section .commit` works on tags in releases list, whether open or collapsed, avoiding the "dummy release" uncollapser element because that doesn't include a `.commit` element.

Tested on https://github.com/facebook/react/releases?after=v16.7.0

@bfred-it bfred-it self-assigned this May 10, 2019

@bfred-it bfred-it removed their assignment May 10, 2019

@sindresorhus
Copy link
Owner

left a comment

Works great 👍

@bfred-it bfred-it merged commit bcb9ed2 into sindresorhus:master May 11, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bfred-it

This comment has been minimized.

Copy link
Collaborator

commented May 11, 2019

Hallelujah! Finally we merged it. Thanks for this feature!

@HardikModha

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

Woahhh, Thanks for refining the feature. ❤️ I just returned today and saw this merged!!! 🎉

But seems like there is a small issue. Tags are not being compared with a namespace.

Screen Shot 2019-05-14 at 8 48 20 PM
It should compare parcel-bundler with it's matching previous tag, not with the parcel-integration. The tag name is not being decoded before extracting version and namespace using parseTags.

@HardikModha

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

Also, I noticed a behavior with version aware comparison. It is comparing the first previous tag and not the closest matching previous release.

e.g. On https://github.com/flutter/flutter/releases?after=v1.5.1 releases page.

Screen Shot 2019-05-14 at 8 59 04 PM

It should compare v1.4.17 with v1.4.16 and not v1.4.9-hotfix.1. What do you think?

@HardikModha HardikModha deleted the HardikModha:add_what_changed_since_prev_release branch May 14, 2019

@bfred-it bfred-it referenced this pull request May 18, 2019

Closed

Refined GitHub updates #1137

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.