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 styling commando's #402

Merged
merged 12 commits into from Apr 8, 2015
Merged

New styling commando's #402

merged 12 commits into from Apr 8, 2015

Conversation

sstok
Copy link
Contributor

@sstok sstok commented Apr 5, 2015

Q A
Fixed Tickets
License MIT

Still a work in progress, but to give an idea :)
Images were edited with Paint so not a full shot ;)

gush-style2
gush-style

Todo:

  • Update all helpers for the new styling
  • Fix CS issues (I forgot to update the headers again...)
  • Update tests

@sstok sstok added the Feature label Apr 5, 2015
@sstok sstok mentioned this pull request Apr 6, 2015
18 tasks
@sstok
Copy link
Contributor Author

sstok commented Apr 6, 2015

Pull request will now report on which org and repository the pull-request will be opened.
That will prevent my most made mistake 😄 (forget using --org)

@sstok
Copy link
Contributor Author

sstok commented Apr 6, 2015

Tests are still broken... will fix them tomorrow.

@cordoval
Copy link
Member

cordoval commented Apr 6, 2015

"still broken" ... hmmm not good not good 👴 😊

public function renderException($e, $output)
{
if ($e instanceof UserException) {
$this->getHelperSet()->get('gush_style')->getStyle()->error($e->getMessage());
Copy link
Member

Choose a reason for hiding this comment

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

Maybe some helper methods like $this->success(...), $this->error(...) etc. will make some of the code less verbose

Copy link
Member

Choose a reason for hiding this comment

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

welcome back boss 👍 👴

Copy link
Member

Choose a reason for hiding this comment

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

😊

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually thought about that :) but that was a bit to much work for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On the other hand, it would make fixing the tests easier :)

When a UserException is thrown the exception is rendered as error-block
instead of an exception-block

The original message is still shown when verbosity is increased
This also fixes that the default value of the issue-tracker didn't work
@sstok
Copy link
Contributor Author

sstok commented Apr 7, 2015

The tests are failing due to a bug in the SymfonyStyle, the console width is empty (null) when running the tests as there is no actual terminal :) and min() is turning that into 0 causing this issue.

Even worse its completely failing when using the Git shell as mode CON is not available then and tty is not working on Windows 😠

Will open an issue for that (if not reported already). issue is already reported with pull.
symfony/symfony#14250

Am signing of for today ;)

@sstok sstok changed the title [WIP] New styling commando's New styling commando's Apr 8, 2015
@sstok
Copy link
Contributor Author

sstok commented Apr 8, 2015

Tests are working, rendering problems are resolved

Ready for merge @cordoval :)

@cordoval
Copy link
Member

cordoval commented Apr 8, 2015

must be a coincidence but @wjzijderveld also has a new github picture i believe. Good job @sstok

$issueNumber
));
$adapter->closeIssue($issueNumber);
if (true === $close ) {
Copy link
Member

Choose a reason for hiding this comment

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

woah! extra space

@cordoval
Copy link
Member

cordoval commented Apr 8, 2015

should i merge now? sounds yummy!

except for LabelIssuesCommand which has to many changes
for a combined commit
@sstok
Copy link
Contributor Author

sstok commented Apr 8, 2015

Just fixing some of the reported issue ;)

- adapt new console styling
- allow empty labels
- fix labels with spaces between commas
- allow using current labels (basically skip)
- remove `--label` option (undocumented and dangerous)
@cordoval
Copy link
Member

cordoval commented Apr 8, 2015

ok, the git history you have seems impressively clean. I wonder if your train of thought was this clear as you progressed or you reordered it in chunks with git add -p after the whole work was finished. Good work man. I can't wait to merge. 👏

@sstok
Copy link
Contributor Author

sstok commented Apr 8, 2015

Doing the commits logically and then doing an interactive rebase ('edit') to fix issues in the original commit 😋

@cordoval
Copy link
Member

cordoval commented Apr 8, 2015

thanks @kbond for this symfony/symfony#14057, this wouldn't be possible were it not for your efforts. Thanks @pierredup for your excellent review. Thanks @javiereguiluz for the great ideas and diligence.

MetaHeaderCommand now gives a much nicer detailed
output (telling which files are actually changed)
And add little information whats going to happen

```
 // This pull-request will be opened on "org/repo".
 // The source branch is "source-branch" on "source-org".
```

And ask confirmation to replace an issue
* fix wrong rendering
* list problems
@sstok
Copy link
Contributor Author

sstok commented Apr 8, 2015

OK, issue are resolved. Ready for merge 👍

Properly my most clean-PR ever 😄

cordoval added a commit that referenced this pull request Apr 8, 2015
@cordoval cordoval merged commit 0a0486b into gushphp:master Apr 8, 2015
@sstok sstok deleted the feat-style branch April 8, 2015 14:26
@cordoval
Copy link
Member

cordoval commented Apr 8, 2015

👍 great work @sstok

@sstok
Copy link
Contributor Author

sstok commented Apr 8, 2015

must be a coincidence but @wjzijderveld also has a new github picture i believe. Good job @sstok

Hehe and I thought I had long hair 😄

@cordoval
Copy link
Member

cordoval commented Apr 8, 2015

I think if things go well this year I will grow long hair too, not that long but maybe a bit. I mean long ⬜ hair as in 🍼 but sign of an 👴

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

Successfully merging this pull request may close these issues.

None yet

3 participants