Skip to content

Comments

Fix unexpected bye by skipping emoji#64

Closed
sanak wants to merge 1 commit intodevelopfrom
fix/unexpected-bye-by-skipping-emoji
Closed

Fix unexpected bye by skipping emoji#64
sanak wants to merge 1 commit intodevelopfrom
fix/unexpected-bye-by-skipping-emoji

Conversation

@sanak
Copy link
Collaborator

@sanak sanak commented Mar 1, 2023

Fixes #61.

Changes proposed in this pull request:

@Georepublic/maintainer

@sanak sanak added the bug Something isn't working label Mar 1, 2023
@sanak sanak self-assigned this Mar 1, 2023
@sanak sanak marked this pull request as ready for review March 1, 2023 07:28
@sanak sanak requested a review from halsk March 1, 2023 07:28
@sanak
Copy link
Collaborator Author

sanak commented Mar 1, 2023

@halsk I confirmed this on dev environment, so could you check this ?
(Note that I changed VERSION directly on Google Apps Script editor.)

@sanak sanak requested a review from Copilot March 12, 2025 03:59
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes the issue of an unexpected bye by ensuring that emoji are removed from messages before commands are identified. It also adds a test case to cover messages that include emojis.

  • Removes emojis from the input message using a regular expression.
  • Updates command matching logic to use the emoji-stripped message.
  • Adds a test case to verify that messages with emoji are handled correctly.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
scripts/timesheets.js Removes emoji from messages for command detection
tests/timesheets_test.js Adds a test case to verify functionality with emoji
Comments suppressed due to low confidence (1)

scripts/timesheets.js:43

  • Review the regex used for emoji removal to ensure it doesn't unintentionally strip parts of valid commands containing colons. Consider adding unit tests for edge cases.
var emojiRemovedMessage = message.replace(/:[^:\s]*(?:::[^:\s]*)*:/g, '');

});

// 休憩時間(稼働中、絵文字付き)
test1 = {};
Copy link

Copilot AI Mar 12, 2025

Choose a reason for hiding this comment

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

Declare 'test1' using let or const to avoid polluting the global scope and prevent potential unintended side effects.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the upper part (line 168 and 176), var test1 = {}; are used.
Is it okay to replace this line to var test1 = {}; ?
line 183 and 197 are also same.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, I used var test1 = {}; in commit 43dc8f8.

@sanak sanak requested a review from Copilot March 12, 2025 04:13

This comment was marked as duplicate.

@sanak sanak changed the base branch from master to develop April 3, 2025 14:25
@sanak
Copy link
Collaborator Author

sanak commented Apr 3, 2025

I missed to set target branch. I already merged 3 commits from this PR branch to develop branch,
so I will merge last 1 commit manually from GitHub Copilot advise.

@sanak sanak closed this Apr 3, 2025
@sanak sanak mentioned this pull request Apr 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

“:curry:” emoji is detected as “bye”

1 participant