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

Template, support optional argument #2102

Closed
wants to merge 2 commits into from
Closed

Template, support optional argument #2102

wants to merge 2 commits into from

Conversation

sukrosono
Copy link

@sukrosono
Copy link
Author

Of the topic,

  1. is that the optional argument will always be placed after the mandatory one?
  2. it changes the clean template, does it change the default template?
    Also it untested, please guide me to properly testing it (if it necessary)

@jaapio , i think we agreed that we are going to changes the default template, right?

@jaapio
Copy link
Member

jaapio commented Jul 30, 2019

Thanks for this PR,

In php terms an optional argument MUST be placed after all mandatory ones. You cannot simply omit a mandatory argument after an optional. function fooBar($optional = 'default', $required) {} cannot be called without errors.

The clean template is the default one.
At this moment we don't have any tests for templates since they are quite hard to write. There are a few of them but they are not working properly.

Could you please rebase this PR so only your changes are included?

@sukrosono
Copy link
Author

sukrosono commented Jul 30, 2019

Sure, captain @jaapio .
It would be a shame if the template doesn't change as we expect.
what i intent is manually test these changes since I don't have many experiences on composer.
I want to test it against my project. before it merged.
But i have no idea how to do it, i never release a project,
but really excited to know how the process involved.

Snap the rebase not clean as we expected, is this still a prob?
maybe i need your aid to rebase 😰

jaapio and others added 2 commits July 30, 2019 21:03
Add release step

Fix phpstan errors
@sukrosono sukrosono closed this Aug 1, 2019
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.

None yet

2 participants