Skip to content

Code quality#329

Merged
vladar merged 12 commits intowebonyx:0.12.xfrom
mcg-web:code_quality
Aug 28, 2018
Merged

Code quality#329
vladar merged 12 commits intowebonyx:0.12.xfrom
mcg-web:code_quality

Conversation

@mcg-web
Copy link
Copy Markdown
Contributor

@mcg-web mcg-web commented Aug 23, 2018

Adding some tools to help maintainers.

@vladar
Copy link
Copy Markdown
Member

vladar commented Aug 23, 2018

Seet, thanks! But can you please change the target branch from 0.12.x to master? I don't think we'll do much work in 0.12.x branch anymore (just minor fixes).

},
"config": {
"bin-dir": "bin",
"platform": {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Remove this to test lib in environment without faking platform.

@mcg-web
Copy link
Copy Markdown
Contributor Author

mcg-web commented Aug 23, 2018

@vladar 0.12 is break for php 7.3 nightly and platform config was hiding this. That the reason why I added this on 0.12 because this helps to fix some issues.

@vladar
Copy link
Copy Markdown
Member

vladar commented Aug 23, 2018

Yeah, but those fixes will stay in 0.12.x, or do you plan to merge them later to master? Master is just quite far away from 0.12.x branch, so there will be lots of conflicts during the merge.

@mcg-web
Copy link
Copy Markdown
Contributor Author

mcg-web commented Aug 23, 2018

0.12.x should always be merged in master, this is the best way to maintain both branch while 0.12 is still maintained, yes I will merged it in master.

@mcg-web mcg-web changed the title [WIP] Code quality Code quality Aug 23, 2018
},
"scripts": {
"static-analysis": [
"rm phpstan.phar || true",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

why not simply use phpstan as a dev dependency? this looks unnecessarily complicated

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

cause phpstan require php 7.1 ;)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This lib does too, hm 🤔?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I m not on master ;) but 0.12.x

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ah, so this can be safely replace with clean dep dependency after rebase onto master, right? #329 (comment)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes this is it 👍

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Not sure if I understand correctly :D This is how it should look like

https://github.com/doctrine/dbal/blob/master/composer.json#L23

And in travis, simply use script: vendor/bin/phpstan analyse

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This PR is targeting 0.12.x we'll see how to clean it for master when it will be merged. Right now lets just focus on this PR please ;)

},
"scripts": {
"static-analysis": [
"rm phpstan.phar || true",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Not sure if I understand correctly :D This is how it should look like

https://github.com/doctrine/dbal/blob/master/composer.json#L23

And in travis, simply use script: vendor/bin/phpstan analyse

@mcg-web
Copy link
Copy Markdown
Contributor Author

mcg-web commented Aug 28, 2018

@vladar can you please review this and tell me if I need to split this in two PR. I need to fix nightly build for the bundle ;)

@vladar vladar merged commit f1fc5d6 into webonyx:0.12.x Aug 28, 2018
@mcg-web mcg-web deleted the code_quality branch August 28, 2018 07:54
@mcg-web
Copy link
Copy Markdown
Contributor Author

mcg-web commented Aug 28, 2018

Thank you I'm working on a PR to merge these changes in master?

@vladar
Copy link
Copy Markdown
Member

vladar commented Aug 28, 2018

Yes, please

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.

3 participants