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

Change parse mode #16

Merged
merged 9 commits into from Apr 10, 2021
Merged

Conversation

talyguryn
Copy link
Member

  • use HTML because it is simpler to encode correctly
  • message design improvements

@talyguryn talyguryn changed the base branch from master to improvements_note_only_data April 7, 2021 22:07
src/index.js Outdated
@@ -113,7 +148,7 @@ function pullRequestParser(content) {
* @returns {string} - parsed message.
*/
function issuesParser(content) {
let parsedTask = `[${content.title.replace(/[[\]']+/g, '')}](${content.url})`;
let parsedTask = `${createTaskBadge(content.url)}: <a href="${content.url}">${escapeChars(content.title, '')}</a>`;
Copy link
Member

Choose a reason for hiding this comment

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

Why escapeChars contain two arguments.

Copy link
Member Author

Choose a reason for hiding this comment

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

oh. sorry. have not refactor. i'll fix

src/index.js Outdated
Comment on lines 314 to 316
if (!items.tasks.length) {
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

@talyguryn, Peter told me earlier to remove this, So we can identify a person without a task.

Copy link
Member Author

Choose a reason for hiding this comment

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

hm. well then we can mark these people other way. maybe put them at the end of the message and mark as "no task" or any other way.

i did this because i see big message which has only 50% useful information. let's discuss a better design format.

Comment on lines +129 to +139
// content.reviewRequests.nodes.forEach((node) => {
// if (node.requestedReviewer.login) {
// parsedTask += `@${node.requestedReviewer.login}`;
// }
// });
//
// content.assignees.nodes.forEach((node) => {
// if (node.login) {
// parsedTask += `@${node.login}`;
// }
// });
Copy link
Member

Choose a reason for hiding this comment

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

Why we removed the requested reviewer and assignees

Copy link
Member Author

Choose a reason for hiding this comment

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

we can discuss it too. i think it is now necessary to duplicate link to pr. u can see repo name and request name and if it is project which u working for u should check it

Comment on lines +98 to +100
message = message.replace(/</g, '&lt;');
message = message.replace(/>/g, '&gt;');
message = message.replace(/&/g, '&amp;');
Copy link
Member

@robonetphy robonetphy Apr 8, 2021

Choose a reason for hiding this comment

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

@talyguryn I change this because replaceAll throwing me an error

Copy link
Member Author

Choose a reason for hiding this comment

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

does it work correctly for multiple entities?

Copy link
Member

Choose a reason for hiding this comment

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

Yup

@talyguryn talyguryn merged commit 5474843 into improvements_note_only_data Apr 10, 2021
@talyguryn talyguryn deleted the feat/change-parse-mode branch April 10, 2021 12:20
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

3 participants