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

treehouses changelog compare <version> [version] upgrade (fixes #2164) #2169

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

Conversation

JLKwong
Copy link
Member

@JLKwong JLKwong commented Apr 8, 2021

No description provided.

@JLKwong JLKwong requested a review from rjpadilla April 8, 2021 01:25
Copy link
Member

@rjpadilla rjpadilla left a comment

Choose a reason for hiding this comment

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

So I don't know about you, but I kept getting Error: file does not exist!.

So I pretty much made a copy of the CHANGELOG.MD from /usr/lib/node_modules/@treehouses/cli/CHANGELOG.md into my repo to get it running.


Moving on to the comparing:
When I run changelog compare 1.25.37 1.25.36 or vice-versa I get the output

Try 'cut --help' for more information. 
ERROR: Must specify different versions for comparisons (cannot compare same version to itself)

I wouldn't know how to fix that problem...


So, my idea of fixing the comparing version issue was using a test like this (but this uses ASCII, so comparing isn't guarantee):

[[ 1.25.37 > 1.25.36 ]]

or some implemetation using the sort command:

echo -e "1.25.37\n1.25.36" | sort -V

Finally you can go with the @dogi lazy option:

dpkg --compare-versions "1.0" "lt" "1.2"

@JLKwong
Copy link
Member Author

JLKwong commented Apr 10, 2021

So I don't know about you, but I kept getting Error: file does not exist!.

So I pretty much made a copy of the CHANGELOG.MD from /usr/lib/node_modules/@treehouses/cli/CHANGELOG.md into my repo to get it running.

Moving on to the comparing:
When I run changelog compare 1.25.37 1.25.36 or vice-versa I get the output

Try 'cut --help' for more information. 
ERROR: Must specify different versions for comparisons (cannot compare same version to itself)

I wouldn't know how to fix that problem...

So, my idea of fixing the comparing version issue was using a test like this (but this uses ASCII, so comparing isn't guarantee):

[[ 1.25.37 > 1.25.36 ]]

or some implemetation using the sort command:

echo -e "1.25.37\n1.25.36" | sort -V

Finally you can go with the @dogi lazy option:

dpkg --compare-versions "1.0" "lt" "1.2"

These bugs have been fixed. 😌✨

image

Copy link
Member

@rjpadilla rjpadilla left a comment

Choose a reason for hiding this comment

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

jamiecomparechangelog

The changelog compare still shows the changes between new version first and old version last. I don't know if the requirements are strict with only comparing old first and new last. But I'll probably leave out the changes and keep the "Did you mean..." line.

Anyways it looks a lot better than showing all the changes.

Comment on lines 2 to 6
if [ -d "tests" ]; then
cp "/usr/lib/node_modules/@treehouses/cli/CHANGELOG.md" .
else
cp "/usr/lib/node_modules/@treehouses/cli/CHANGELOG.md" ../.
fi
Copy link
Member

Choose a reason for hiding this comment

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

I may be mistaken but I think @dogi left checking the directory for tests/changelog.bats
Also you didn't remove the CHANGELOG file....

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed - it now removes the duplicate CHANGELOG file.

Copy link
Member Author

Choose a reason for hiding this comment

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

jamiecomparechangelog

The changelog compare still shows the changes between new version first and old version last. I don't know if the requirements are strict with only comparing old first and new last. But I'll probably leave out the changes and keep the "Did you mean..." line.

Anyways it looks a lot better than showing all the changes.

Are you saying you want an error handling message when when comparing changes with the new version first and old version last?

And you're correct, the way the code is written now, the command will 'autocorrect' by displaying the changes as if the versions were switched around, then suggest the correct way to write the command:
image

@JLKwong JLKwong requested a review from rjpadilla April 28, 2021 09:14
@JLKwong
Copy link
Member Author

JLKwong commented Apr 28, 2021

treehouses changelog compare will now echo the version differences regardless of whether and older or newer version is specified first or last.

Copy link
Member

@rjpadilla rjpadilla left a comment

Choose a reason for hiding this comment

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

So it works...
but the CHANGELOG.md is still in the cli/ directory and you removed the CHANGELOG.md from my treehouses image.

jamiedeletedchangelog

Comment on lines 2 to 8
if [ -d "tests" ]; then
cp -f "/usr/lib/node_modules/@treehouses/cli/CHANGELOG.md" . 2>/dev/null || true
rm -f "/usr/lib/node_modules/@treehouses/cli/CHANGELOG.md"
else
cp -f "/usr/lib/node_modules/@treehouses/cli/CHANGELOG.md" ../. 2>/dev/null || true
rm -f "/usr/lib/node_modules/@treehouses/cli/CHANGELOG.md"
fi
Copy link
Member

Choose a reason for hiding this comment

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

You can delete this. This belongs in the changelog.bats

Copy link
Member Author

Choose a reason for hiding this comment

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

You can delete this. This belongs in the changelog.bats

I deleted the code you mentioned.

@JLKwong JLKwong requested a review from rjpadilla May 7, 2021 05:15
Copy link
Member

@rjpadilla rjpadilla left a comment

Choose a reason for hiding this comment

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

My changelog is gone so I can't thoroughly test. But from the previous PR, it seems it works except for the block of code that removed my changelog. Now that you removed it, I think everything is good.

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