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

Remove hash from hashtag list #1462

Merged
merged 1 commit into from
Aug 16, 2017
Merged

Conversation

singhpratyush
Copy link
Member

Short description

Fixes #1457.

I have:

  • There is a corresponding issue for this pull request.
  • Mentioned the Issue number in the pull request commit message Fixes #<number> commit message
  • There is only strictly only one commit per issue.

For the reviewers

I have:

  • Reviewed this pull request by an authorized contributor.
  • The reviewer is assigned to the pull request.

@codecov-io
Copy link

codecov-io commented Aug 16, 2017

Codecov Report

Merging #1462 into development will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@              Coverage Diff              @@
##             development   #1462   +/-   ##
=============================================
  Coverage           9.02%   9.02%           
  Complexity           403     403           
=============================================
  Files                203     203           
  Lines              18086   18086           
  Branches            3325    3325           
=============================================
  Hits                1633    1633           
  Misses             16131   16131           
  Partials             322     322
Impacted Files Coverage Δ Complexity Δ
src/org/loklak/objects/MessageEntry.java 35.82% <100%> (ø) 17 <0> (ø) ⬇️

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 6c7e562...53a530f. Read the comment docs.

@singhpratyush
Copy link
Member Author

singhpratyush commented Aug 16, 2017

@@ -57,7 +57,7 @@
// left boundary must be space since the @ is itself a boundary
public final static Pattern USER_PATTERN = Pattern.compile("(?:[ (]|^)(@..*?)(?:\\b|$)");
// left boundary must be a space since the # is itself a boundary
final static Pattern HASHTAG_PATTERN = Pattern.compile("(?:[ (]|^)(#..*?)(?:\\b|$)");
final static Pattern HASHTAG_PATTERN = Pattern.compile("(?:[ (]|^)#(..*?)(?:\\b|$)");
Copy link
Member

Choose a reason for hiding this comment

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

Any tests? Doesn't keeping the # outside mandate that the hash is available in the pattern?

It'd always be better to have a test to pass and fail these patterns, that'd give a detailed reason as to why this change in regex is needed.

Copy link
Member

@vibhcool vibhcool Aug 16, 2017

Choose a reason for hiding this comment

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

@sudheesh001 There isn't a bug in hashtags, the hashtags work fine, but Loklak Search team suggested that hashtags strings are more useful than hashtags with # symbol. #1457 (comment)
EDIT: yes, we need to add more detailed tests 😅

@mariobehling mariobehling merged commit 579dbd9 into loklak:development Aug 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants