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

Added StringOperatorType #6134

Merged
merged 7 commits into from
Jun 27, 2020
Merged

Added StringOperatorType #6134

merged 7 commits into from
Jun 27, 2020

Conversation

napestershine
Copy link
Contributor

@napestershine napestershine commented Jun 9, 2020

Subject

Provides fix for:
sonata-project/SonataDoctrineORMAdminBundle#943 (comment)

I am targeting this branch, because its a new feature.

Provides fix for:
sonata-project/SonataDoctrineORMAdminBundle#943 (comment)

Changelog

### Added
- Added `StringOperatorType`

@napestershine
Copy link
Contributor Author

Can you please release this when it get merged. Thanks

VincentLanglet
VincentLanglet previously approved these changes Jun 9, 2020
@VincentLanglet VincentLanglet requested a review from a team June 9, 2020 19:28
Copy link
Contributor

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

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

Please format your commit message according to our rules :

  • The commit message subject must be less than 50 characters and tell us what you did.
  • The commit message body should tell us why you did it. It is optional but highly recommended.

Bad example :

Fixed bug #989 by removing call to defraculate()

Good example:

Remove call to defraculate()

Calling this function caused a bug because it interferes 
with calls to getSchmeckles().
Fixes #989

More details in CONTRIBUTING.md

@napestershine
Copy link
Contributor Author

@greg0ire I am sorry. Not sure how to do that. I just made changes on github only. Don't have a local copy.

Can someone please add some steps to get it done ?

TIA

@VincentLanglet
Copy link
Member

@greg0ire I am sorry. Not sure how to do that. I just made changes on github only. Don't have a local copy.

Can't be done without a local copy. You'll need it.
Did you ever work with local repository ?

@greg0ire I can reformat the commit message when using "Squash merge". Would it be enough ?

@napestershine
Copy link
Contributor Author

I can work with git. But not sure which one to clone my fork or sonata one. and then next steps ?

@VincentLanglet
Copy link
Member

I can work with git. But not sure which one to clone my fork or sonata one. and then next steps ?

  1. Clone your fork.
  2. Checkout on your branch patch-1.
  3. Then you can commit amend, rebase, etc to modify your commits.
  4. Push force to your repository.

@napestershine napestershine changed the title Update ContainsOperatorType.php Added new Added StringOperatorType Jun 11, 2020
@napestershine
Copy link
Contributor Author

@VincentLanglet done. Thanks for guiding.

@napestershine napestershine changed the title Added new Added StringOperatorType Added new StringOperatorType Jun 11, 2020
@VincentLanglet
Copy link
Member

Nice, can you rename your commit "Add StringOperatorType".

Because "Update ContainsOperatorType.php" is not what you're doing in this commit.

@napestershine napestershine changed the title Added new StringOperatorType Added StringOperatorType Jun 11, 2020
@napestershine
Copy link
Contributor Author

What else is left for this? Or it's ready?

@jordisala1991
Copy link
Member

Adding some tests would be nice

@napestershine
Copy link
Contributor Author

Can you point me to a sample test or something related to it already implemented that I can follow?

TIA

@jordisala1991

@VincentLanglet
Copy link
Member

@napestershine There is currently no test for OperatorType.

@jordisala1991 For what the form is doing, I'm not sure it's really useful

VincentLanglet
VincentLanglet previously approved these changes Jun 11, 2020
VincentLanglet
VincentLanglet previously approved these changes Jun 23, 2020
@VincentLanglet VincentLanglet requested a review from a team June 23, 2020 16:13
core23
core23 previously approved these changes Jun 23, 2020
@napestershine
Copy link
Contributor Author

Is it possible to merge it. Need it in a project. TIA

@VincentLanglet
Copy link
Member

@greg0ire Is it ok for you ?

@napestershine napestershine dismissed stale reviews from core23 and VincentLanglet via a570e44 June 25, 2020 08:27
@napestershine
Copy link
Contributor Author

@VincentLanglet those failed checks are not related to this PR

@VincentLanglet
Copy link
Member

@VincentLanglet those failed checks are not related to this PR

Indeed, but we can't merge anything with failing tests.
We're currently working on it. #6168

@greg0ire
Copy link
Contributor

greg0ire commented Jun 26, 2020

@greg0ire Is it ok for you ?

There are 7 commits, none of which have a meaningful message, so… no?

How to squash your commits?

  1. git rebase -i origin/3.x, assuming origin is a git remote that points to this repository, and not your fork. If you're not sure what your remotes are, run git remote -vvv, there should be your fork and the holy/reference/base/origin/whatever-you-call-it repository.
  2. A window will show up with many lines, replace pick with fixup on every line but the first one
  3. Close your editor, git should do its magic, and you should end up with one commit
  4. Use git push --force to overwrite what you already push. Don't forget the --force option otherwise git will try to merge both things together.

Also, this is a new feature… shouldn't it come with some docs?

@VincentLanglet
Copy link
Member

@greg0ire Is it ok for you ?

There are 7 commits, none of which have a meaningful message, so… no?

It didn't bother me to squash-and-merge and change the message to a more meaningful one.

Also, this is a new feature… shouldn't it come with some docs?

We don't have doc about OperatorTypes.
OperatorTypes are mainly created to be used in the filter.
This one will be used to improve the StringFilter.

@greg0ire greg0ire merged commit ba2f7cc into sonata-project:3.x Jun 27, 2020
@greg0ire
Copy link
Contributor

Ok then… the thing is with the squash and merge is… you have to not forget to do it. And when there are pending issues to resolve first, forgetting becomes more and more likely.

@napestershine
Copy link
Contributor Author

Sure. Thanks so much

@jordisala1991
Copy link
Member

The changelog do not follow the same format as the provided template, this could cause issues when generating the changelog @greg0ire

@greg0ire
Copy link
Contributor

Indeed, fixed.

@napestershine
Copy link
Contributor Author

Can it be released please when possible, so I can finish other PR sonata-project/SonataDoctrineORMAdminBundle#1057

@VincentLanglet
Copy link
Member

Released in 3.71. @napestershine

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.

7 participants