[go: nahoru, domu]

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

Add additional tests for RegEx #81742

Merged

Conversation

toastedbreadandomelette
Copy link
Contributor

This PR should hopefully address more tests for RegEx. This task is part of #43440.

More Details:

modules/regex/tests/test_regex.h Outdated Show resolved Hide resolved
modules/regex/tests/test_regex.h Outdated Show resolved Hide resolved
modules/regex/tests/test_regex.h Outdated Show resolved Hide resolved
@AThousandShips
Copy link
Member

Also please squash your commits into one, see here

Make sure you push your changes with git push --force

@AThousandShips
Copy link
Member

You erased your previous fixes, please add them back in and squash all commits into one

@toastedbreadandomelette toastedbreadandomelette force-pushed the regex_additional_tests branch 2 times, most recently from d811ea0 to c4fede7 Compare September 16, 2023 15:42
@toastedbreadandomelette
Copy link
Contributor Author

These changes should be as per the suggestions. Let me know if there are more changes.

@inquisitev
Copy link

You erased your previous fixes, please add them back in and squash all commits into one

why ask developers to manually squash into one commit when github can do it for you on merge of PR by checking a box under settings?

@AThousandShips
Copy link
Member

Because we don't do those features, they add their own artifacts and can create issues as far as I know

@inquisitev
Copy link

Because we don't do those features, they add their own artifacts and can create issues as far as I know

interesting, we use that feature at work and i have not seen any issues, but ill take your word for it

@Calinou
Copy link
Member
Calinou commented Sep 16, 2023

We don't use GitHub's squash and merge feature on the main Godot repository, since it loses the merge commit in the process. We prefer keeping the merge commit to make the Git history more self-documented (even if that makes it longer when not using the --no-merges option).

Copy link
Member
@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally (rebased on top of master 83b916b), it works as expected.

I force-pushed a rebased version with the commit message modified to follow conventions used in the repository, so this should be good to merge now.

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Apr 10, 2024
@akien-mga akien-mga merged commit 5a38628 into godotengine:master Apr 11, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

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

Successfully merging this pull request may close these issues.

None yet

5 participants