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

feat: bump dev-dependencies #775

Closed
wants to merge 5 commits into from
Closed

feat: bump dev-dependencies #775

wants to merge 5 commits into from

Conversation

Chris53897
Copy link
Contributor

Copy link
Member

@frederikbosch frederikbosch left a comment

Choose a reason for hiding this comment

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

Nice that you directly also take-up this one.

composer.json Outdated Show resolved Hide resolved
@frederikbosch
Copy link
Member

This might be much more work than you anticipated, because the static analyzers might have quite something to complain.

@frederikbosch
Copy link
Member

That things with that comma added to every argument list, can you disable that? Personally, I do not see why we want that. I makes this PR really hard to check also.

@Chris53897
Copy link
Contributor Author

The rules need to be changed here. But i am not which one to add here.
https://github.com/moneyphp/money/blob/master/phpcs.xml.dist

I am not yet sure what exactly does cause the changes for PHPCSFixer to find, all the new violations.
https://github.com/moneyphp/money/actions/runs/7665540242/job/20891619569

@frederikbosch
Copy link
Member

frederikbosch commented Jan 26, 2024

It is caused by the upgrade of doctrine/coding-standard. To be honest, I have no idea either which config it is. I looked up the docs but I am not sure actually.

@frederikbosch
Copy link
Member

Can you try to exclude SlevomatCodingStandard.Functions.RequireTrailingCommaInCall and SlevomatCodingStandard.Functions.RequireTrailingCommaInClosureUse?

@Chris53897
Copy link
Contributor Author

I think splitting it up to seperate PR's make it easier.
Maybe you can have a look at the other two PR's from me.
After that is merged, #746 could be rebased against master.
And hopefully finalised, and merged.

After that is done a new PR for the coding standards could be done.
WDYT ?

@frederikbosch
Copy link
Member

Ok. Let's do the coding standards upgrade here then.

@frederikbosch
Copy link
Member

Superseded by #785

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