Skip to content
This repository was archived by the owner on Nov 1, 2022. It is now read-only.

Issue #5376: Simpler method for determining whether a string is a search query #5384

Merged
merged 1 commit into from
Jan 3, 2020
Merged

Issue #5376: Simpler method for determining whether a string is a search query #5384

merged 1 commit into from
Jan 3, 2020

Conversation

hawkinsw
Copy link
Contributor

Update URLStringUtils.isSearchTerm to use a simpler method for
determining whether a string is a search query or a URL.

Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry or does not need one
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

  • Milestone: Make sure issues closed by this pull request are added to the milestone of the version currently in development.
  • Breaking Changes: If this is a breaking change, please push a draft PR on Reference Browser to address the breaking issues.

@codecov
Copy link

codecov bot commented Dec 20, 2019 •
edited
Loading

Codecov Report

Merging #5384 into master will decrease coverage by 0.14%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master   #5384      +/-   ##
===========================================
- Coverage     80.34%   80.2%   -0.15%     
+ Complexity     4241    4195      -46     
===========================================
  Files           547     537      -10     
  Lines         19267   19045     -222     
  Branches       2785    2765      -20     
===========================================
- Hits          15481   15275     -206     
+ Misses         2621    2609      -12     
+ Partials       1165    1161       -4
Impacted Files Coverage Δ Complexity Δ
...mozilla/components/support/utils/URLStringUtils.kt 100% <100%> (ø) 10 <5> (+2) ⬆️
...va/mozilla/components/support/ktx/kotlin/String.kt 94.11% <100%> (+0.17%) 0 <0> (ø) ⬇️
...ain/java/mozilla/components/service/glean/Glean.kt
...ponents/feature/tabs/toolbar/TabsToolbarFeature.kt
...ents/service/glean/net/ConceptFetchHttpUploader.kt
...la/components/feature/tabs/tabstray/TabsFeature.kt
...ponents/feature/tabs/tabstray/TabsTrayPresenter.kt
...a/mozilla/components/feature/tabs/WindowFeature.kt
...onents/feature/tabs/tabstray/TabsTrayInteractor.kt
...ts/feature/tabs/toolbar/TabCounterToolbarButton.kt
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8be7ece...4af24d9. Read the comment docs.

@mcomella mcomella added the 🕵️‍♀️ needs review PRs that need to be reviewed label Dec 20, 2019
mcomella
mcomella reviewed Dec 20, 2019
View reviewed changes
Copy link
Contributor

@mcomella mcomella left a comment

Choose a reason for hiding this comment

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

I'll leave it up to a-c to do the final review. I would need more time to review this if I was the final reviewer (specifically, is the algorithm sufficient?) but for the general concept I'm leaning towards r+ once the unicode issue is addressed.

components/support/utils/src/main/java/mozilla/components/support/utils/URLStringUtils.kt
return uri.toString()
}

private val isURLLenient by lazy {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll leave this up to the lib owners but I think we should remove isURLLike or at least have it share the isSearchTerm impl because:

  • isURLLike seems redundant to isSearchTerm
  • What is isURLLike supposed to mean? Without further details, how are consumers supposed to use it confidently? Note that it doesn't look like our code uses it (query)

@hawkinsw hawkinsw changed the title Issue #5376: Simpler method for determing whether a string is a search query Issue #5376: Simpler method for determining whether a string is a search query Dec 20, 2019
@hawkinsw hawkinsw requested a review from mcomella December 21, 2019 07:26
mcomella
mcomella reviewed Dec 26, 2019
View reviewed changes
Copy link
Contributor

@mcomella mcomella left a comment

Choose a reason for hiding this comment

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

wfm but I'll hold off approving in favor of someone from a-c to review.

mcomella
mcomella reviewed Dec 26, 2019
View reviewed changes
components/support/utils/src/main/java/mozilla/components/support/utils/URLStringUtils.kt Outdated
}

fun isSearchTerm(string: String) = !isURLLike(string, false)
fun isSearchTerm(string: String) = !isURLLenient.matcher(string).matches()
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably needs a changelog entry.

mcomella
mcomella reviewed Dec 26, 2019
View reviewed changes
Copy link
Contributor

@mcomella mcomella left a comment

Choose a reason for hiding this comment

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

Again, seems reasonable to me but waiting for a-c review.

@mcomella mcomella mentioned this pull request Dec 30, 2019
Closed
4 tasks
@csadilek csadilek self-assigned this Dec 31, 2019
@hawkinsw hawkinsw requested review from csadilek and mcomella January 2, 2020 06:03
@hawkinsw
Copy link
Contributor Author

hawkinsw commented Jan 2, 2020

Do not review this PR until further notice. There is a discrepancy in compiling regular expressions that work on the host (the developer's computer, that is) and that work on the device.

@mcomella
Copy link
Contributor

mcomella commented Jan 2, 2020

Do not review this PR until further notice. There is a discrepancy in compiling regular expressions that work on the host (the developer's computer, that is) and that work on the device.

Could we compile regex that work on device and, for now, not worry about those on the host? We could run our tests on device and not run the tests as unit tests. This assumes a-c has tests on device (which they should! 😄)

@mcomella
Copy link
Contributor

mcomella commented Jan 2, 2020

Removing review request: I believe a-c will take it from here.

@mcomella mcomella removed their request for review January 2, 2020 16:02
csadilek
csadilek approved these changes Jan 3, 2020
View reviewed changes
Copy link
Contributor

@csadilek csadilek left a comment

Choose a reason for hiding this comment

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

👍

Thanks, @hawkinsw.

@hawkinsw
Issue #5376: Lenient and strict methods for determing whether a strin…
4af24d9 
…g is a URL

From only one method to determine whether a string is a url, create
two -- one for a strict check and one for a lenient check. The strict
check will guarantee that the URL precisely conforms to ICANN standards
(e.g., the TLD in the URL is valid). The lenient check will classify any
string that contains ://, ., or : as a URL.
@csadilek
Copy link
Contributor

csadilek commented Jan 3, 2020

bors r+

bors bot pushed a commit that referenced this pull request Jan 3, 2020
@MozLando @hawkinsw
Merge #5384
287b257 
5384: Issue #5376: Simpler method for determining whether a string is a search query r=csadilek a=hawkinsw

Update URLStringUtils.isSearchTerm to use a simpler method for
determining whether a string is a search query or a URL.




Co-authored-by: Will Hawkins <whh8b@obs.cr>
@bors
Copy link

bors bot commented Jan 3, 2020

Build succeeded

  • complete-push

@bors bors bot merged commit 4af24d9 into mozilla-mobile:master Jan 3, 2020
@csadilek csadilek mentioned this pull request Jan 3, 2020
Closed
@hawkinsw hawkinsw mentioned this pull request Jan 6, 2020
Closed
@cadeyrn cadeyrn mentioned this pull request Jan 23, 2020
Closed
@acreskeyMoz acreskeyMoz mentioned this pull request Feb 10, 2020
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
2 more reviewers

@mcomella mcomella mcomella left review comments

@csadilek csadilek csadilek approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@csadilek csadilek

Labels
🕵️‍♀️ needs review PRs that need to be reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hawkinsw @mcomella @csadilek