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

New commands, bug fixes and support for aliases #54

Merged
merged 20 commits into from
Aug 16, 2017
Merged

New commands, bug fixes and support for aliases #54

merged 20 commits into from
Aug 16, 2017

Conversation

pomek
Copy link
Member

@pomek pomek commented Aug 4, 2017

Suggested merge commit message (convention)

Feature: Introduced new commands and fixes bugs related to invalid displayed errors. Closes #2. Closes #45. Closes #49. Closes #52.


Additional information

On the occasion, I simplified the promises flow in the commands.

@pomek pomek requested a review from Reinmar August 4, 2017 14:55
@pomek
Copy link
Member Author

pomek commented Aug 4, 2017

image

image

So we need to restart the build after some time.

README.md Outdated
@@ -98,6 +98,12 @@ CLI options:
--scope Restricts the command to packages which names match the given glob pattern.

Default: null

--packages-prefix The common name of the packages. The prefix will be removed from packages' names in order to
Copy link
Member

Choose a reason for hiding this comment

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

Why is this a CLI config option? Do you expect anyone to use it? I don't. If anywhere – this is for mgit.json only.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed in d95291a.

@Reinmar
Copy link
Member

Reinmar commented Aug 9, 2017

Since you're adding the checkout command, I think that we should also clean up the bootstrap/update mess. It would be good to decide what commands we use and how and when.

@pomek
Copy link
Member Author

pomek commented Aug 9, 2017

Could we clean up the mess after merging these changes? I would like to avoid bigger PR than we have now. After merging we will be able to decide how these commands should work and what should do.

@Reinmar
Copy link
Member

Reinmar commented Aug 9, 2017

Yes, we can. But if you wanted to add mgit status we should've bootstrap this discussion before. DOn't dive into topics which are not yet clarified because you may be wasting your time.

README.md Outdated
@@ -255,6 +254,62 @@ Example:
mgit save-hashes
```

### status
##### Available also `st`
Copy link
Member

Choose a reason for hiding this comment

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

That shouldn't be a heading. Don't use semantical markup for styling reasons.

Copy link
Member

Choose a reason for hiding this comment

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

We can have ### stats (alias: st).

Please also add a screenshot.

README.md Outdated
In order to save space in your terminal, you can define the `packagesPrefix` option in your configuration file.
The prefix will be removed from packages' names. Full names of packages aren't needed so we can cat the names.

### diff
Copy link
Member

Choose a reason for hiding this comment

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

Please add a screenshot.

@@ -21,57 +21,55 @@ module.exports = {
execute( data ) {
const log = require( '../utils/log' )();

return new Promise( ( resolve, reject ) => {
const destinationPath = path.join( data.options.packages, data.repository.directory );
const destinationPath = path.join( data.options.packages, data.repository.directory );
Copy link
Member

Choose a reason for hiding this comment

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

Did you change anything in this file or is it just a code style?

Copy link
Member

Choose a reason for hiding this comment

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

If so – why changing anything?

Copy link
Member Author

Choose a reason for hiding this comment

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

Changes in bootstrap, savehashes and update will resolve #45 and #49.

execCommand.execute( getExecData( 'git rev-parse HEAD' ) )
.then( execResponse => {
const commitHash = execResponse.logs.info[ 0 ];
return execCommand.execute( getExecData( 'git rev-parse HEAD' ) )
Copy link
Member

Choose a reason for hiding this comment

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

The same question as about bootstrap.

@@ -22,82 +22,77 @@ module.exports = {
const bootstrapCommand = require( './bootstrap' );
const execCommand = require( './exec' );

return new Promise( ( resolve, reject ) => {
const destinationPath = path.join( data.options.packages, data.repository.directory );
const destinationPath = path.join( data.options.packages, data.repository.directory );
Copy link
Member

Choose a reason for hiding this comment

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

The same – what are these changes?

@Reinmar
Copy link
Member

Reinmar commented Aug 9, 2017

I found one issue:

image

@Reinmar
Copy link
Member

Reinmar commented Aug 9, 2017

And the co task could print less info if everything went well. This is hard to parse:

image

One line per repo would be better. Something like "HEAD is now at f1131bc... Merge pull request #33 from ckeditor/t/32" will be fine.

@Reinmar
Copy link
Member

Reinmar commented Aug 9, 2017

Another bug – mgit update leaves repos on detached state. State before and after calling mgit update:

image

This is a regression.

EDIT: My bad. I was on master-revisions.

@pomek
Copy link
Member Author

pomek commented Aug 9, 2017

I found one issue:

Ok, mgit should not show staged files as modified also. I'll correct it.

@pomek
Copy link
Member Author

pomek commented Aug 9, 2017

And the co task could print less info if everything went well. This is hard to parse:

It is also hard to predict what we want to display:

image

Maybe should we display the last line of the log instead of all?

@Reinmar
Copy link
Member

Reinmar commented Aug 9, 2017

Maybe should we display the last line of the log instead of all?

👍

@pomek
Copy link
Member Author

pomek commented Aug 9, 2017

image

@Reinmar
Copy link
Member

Reinmar commented Aug 16, 2017

I've been using these new commands for some time and they are great. No more issues found.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants