Skip to content

Added support for tags#152

Merged
pomek merged 11 commits intomasterfrom
i/148
Dec 2, 2022
Merged

Added support for tags#152
pomek merged 11 commits intomasterfrom
i/148

Conversation

@przemyslaw-zan
Copy link
Copy Markdown
Member

Suggested merge commit message (convention)

Feature: Added support for tags. Closes #148.


Additional information

For example – encountered issues, assumptions you had to make, other affected tickets, etc.

@pomek
Copy link
Copy Markdown
Member

pomek commented Nov 30, 2022

A few notes without scanning the code:

  • It would be good to exclude repositories checked-out tags from push, pull, commit, and close commands.
    • push:
      image
    • pull:
      image
  • What the L symbol stands for?
    image
    • I would go with @ or T (tags). L (latest) does not mean anything in the entire context of the status command.
  • mrgit save produces invalid results now.
     "ckeditor5-dev": "ckeditor/ckeditor5-dev@latest#dccbeb5"
    • mrgit save --branch fails even for repositories with no tags.
    • Everything works fine on master.

@pomek
Copy link
Copy Markdown
Member

pomek commented Dec 1, 2022

mrgit save --branch fails even for repositories with no tags.

It creates the following entry: "ckeditor5-dev": "ckeditor/ckeditor5-dev#HEAD (no branch)" when checked-out tag.

But it can be extracted into a follow-up issue.

execCommand.execute( getExecData( 'git status --branch --porcelain' ) )
execCommand.execute( getExecData( 'git status --branch --porcelain' ) ),
execCommand.execute( getExecData( 'git describe --abbrev=0 --tags' ) ),
execCommand.execute( getExecData( 'git tag --sort=committerdate' ) )
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"mrgit": "https://github.com/cksource/mrgit.git@latest"

image

This is crazy. We need to find a new way to extract the latest tag.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Addressed in 0b9fc08.

Copy link
Copy Markdown
Member

@pomek pomek left a comment

Choose a reason for hiding this comment

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

LGTM.

.then( async () => {
let checkoutValue;

if ( !data.repository.tag ) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This could be potentially extracted to a local function. There is a very similar code starting from line 211.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

They are both just a bit different, making a function out of the two would require bunch of arguments and would make more mess than anything, not worth it IMO

@pomek pomek merged commit be59046 into master Dec 2, 2022
@pomek pomek deleted the i/148 branch December 2, 2022 11:36
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.

Support for tags

3 participants