Skip to content

issue #1052 port MsgBlockParser from TypeScript to Kotlin - #1079

Merged
tomholub merged 1 commit into
masterfrom
ip-1052-msg-block-parser
Mar 12, 2021
Merged

issue #1052 port MsgBlockParser from TypeScript to Kotlin#1079
tomholub merged 1 commit into
masterfrom
ip-1052-msg-block-parser

Conversation

@IvanPizhenko

@IvanPizhenkoIvanPizhenko commented Mar 12, 2021•
edited by tomholub
Loading

Copy link
Copy Markdown
Contributor

// edit tom

close#1052

@IvanPizhenko

IvanPizhenko commented Mar 12, 2021

Copy link
Copy Markdown
ContributorAuthor

@tomholub@DenBond7 Please review this

tomholub
tomholub approved these changes Mar 12, 2021
View reviewed changes

@tomholubtomholub left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good to me. @DenBond7 you should be seeing GitHub UI to also provide a review, since you will better know about Android specific parts.

@tomholub

tomholub commented Mar 12, 2021

Copy link
Copy Markdown
Collaborator

image

@DenBond7

DenBond7 commented Mar 12, 2021

Copy link
Copy Markdown
Collaborator

@tomholub I'm already reviewing it

DenBond7
DenBond7 reviewed Mar 12, 2021
View reviewed changes
Comment threadFlowCrypt/src/test/java/com/flowcrypt/email/core/msg/MsgBlockParserTest.kt

@Test
fun testDetectBlocksIgnoresFalsePositiveBlocks() {
checkForSinglePlaintextBlock(

@DenBond7DenBond7Mar 12, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Maybe it'll be better to store such a long string as resources. Such code looks bloated and a little hard to read it.

@IvanPizhenkoIvanPizhenkoMar 12, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I think it's ok for unit tests, we do it similarly same in the other repos too.

DenBond7
DenBond7 approved these changes Mar 12, 2021
View reviewed changes

@DenBond7DenBond7 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good to me. The Android side was not affected. Maybe need to move long strings to resources. Anyway, I'm approving it.

@IvanPizhenko

IvanPizhenko commented Mar 12, 2021

Copy link
Copy Markdown
ContributorAuthor

@tomholub I can see that there are already 2 approvals and all checks have passed, but I can't merge this myself. Please merge this and possibly give me merge permission under condition of having two approvals.

@tomholub

tomholub commented Mar 12, 2021

Copy link
Copy Markdown
Collaborator

I've set the limit to 1 approval on android repo, and added merge for both Ivan and Den.

That means Den can approve and merge Ivan's work.

We are moving towards a feature per PR - we used to lump many changes in one PR on this repo because Den worked here alone with my oversight. Now that PRs will be smaller, Ivan can also approve/merge Den's work. The permissions are in place.

@tomholub

tomholub commented Mar 12, 2021

Copy link
Copy Markdown
Collaborator

Oh - please always squash merge. Let me add that restriction.

@tomholub
tomholub merged commit e1c60f8 into masterMar 12, 2021
@IvanPizhenko

IvanPizhenko commented Mar 12, 2021

Copy link
Copy Markdown
ContributorAuthor

@tomholub Thanks!

@DenBond7
DenBond7 deleted the ip-1052-msg-block-parser branch March 15, 2021 06:38
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DenBond7DenBond7DenBond7 approved these changes
@tomholubtomholubtomholub approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

port MsgBlockParser from TypeScript to Kotlinpgp message block parser

3 participants

@IvanPizhenko@tomholub@DenBond7