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

Fix stringContainsInOrder to account for repetitions and empty strings #178

Merged
merged 2 commits into from
Aug 11, 2021

Conversation

aliak00
Copy link
Contributor

@aliak00 aliak00 commented Aug 8, 2021

No description provided.

@google-cla
Copy link

google-cla bot commented Aug 8, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no label Aug 8, 2021
@aliak00
Copy link
Contributor Author

aliak00 commented Aug 8, 2021

@googlebot I signed it!

@google-cla google-cla bot added cla: yes and removed cla: no labels Aug 8, 2021
fromIndex = item.indexOf(s, fromIndex);
if (fromIndex < 0) return false;
if (s.isEmpty) continue;
fromIndex = item.indexOf(s, fromIndex) + 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should actually be this probably right?

Suggested change
fromIndex = item.indexOf(s, fromIndex) + 1;
fromIndex = item.indexOf(s, fromIndex) + s.length;

Copy link
Contributor

@jakemac53 jakemac53 Aug 9, 2021

Choose a reason for hiding this comment

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

Oh except that wouldn't work when you get -1. But the current implementation also isn't correct, for instance I think we should have this test:

shouldFail('foo', stringContainsInOrder(['fo', 'oo']));

Which I think would (unexpectedly) pass in the current implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah true. Good catch! Will fix and push update soon!

Comment on lines 125 to 127
if (s.isEmpty) continue;
fromIndex = item.indexOf(s, fromIndex) + 1;
if (fromIndex <= 0) return false;
Copy link
Contributor

@jakemac53 jakemac53 Aug 9, 2021

Choose a reason for hiding this comment

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

How about this?

Suggested change
if (s.isEmpty) continue;
fromIndex = item.indexOf(s, fromIndex) + 1;
if (fromIndex <= 0) return false;
var index = item.indexOf(s, fromIndex);
if (index < 0) return false;
fromIndex = index + s.length;

@google-cla
Copy link

google-cla bot commented Aug 11, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Aug 11, 2021
Copy link
Contributor

@jakemac53 jakemac53 left a comment

Choose a reason for hiding this comment

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

Note that we may have to revert this, at least temporarily, if it causes too much breakage in existing tests in the Dart/Flutter sdks or internal tests.

@jakemac53
Copy link
Contributor

@googlebot I consent

@google-cla google-cla bot added cla: yes and removed cla: no labels Aug 11, 2021
@jakemac53 jakemac53 merged commit 6ba4a6d into dart-lang:master Aug 11, 2021
@aliak00 aliak00 deleted the fix-stringContainsInOrder branch August 11, 2021 14:58
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