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

Br/reply to #17

Merged
merged 12 commits into from
Jul 8, 2019
Merged

Br/reply to #17

merged 12 commits into from
Jul 8, 2019

Conversation

breitman
Copy link
Collaborator

@breitman breitman commented Jul 2, 2019

In test file, the 'from' field says name is null. Not sure why that is, but I matched it in the expect block to let the test pass. It must be due to the beforeEach block that adds the same account without including a name field. I will change it if needed, but I am pretty sure this fix is working correctly.

Copy link
Contributor

@hallettj hallettj left a comment

Choose a reason for hiding this comment

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

I appreciate all of the effort you are putting into this feature! I know that the work has been complicated; I'm pleased with your persistence.

Yes, the name for some addresses will be null. That is because I have not added support for setting a name for an account, so messages sent from Poodle do not have names in the from address.

In the future I would like you to make pull request titles more descriptive. It's a good habit because descriptive titles help code reviewers to understand what they are looking at, and pull requests form a historical record.

There are merge conflicts - you will need to merge changes from master and resolve those conflicts.

packages/main/src/compose/edit.ts Outdated Show resolved Hide resolved
packages/main/src/compose/edit.ts Outdated Show resolved Hide resolved
packages/main/src/compose/reply.ts Outdated Show resolved Hide resolved
packages/main/src/models/Address.ts Show resolved Hide resolved
packages/main/src/models/conversation.ts Outdated Show resolved Hide resolved
packages/main/src/resolvers/conversation.test.ts Outdated Show resolved Hide resolved
packages/main/src/resolvers/conversation.test.ts Outdated Show resolved Hide resolved
packages/main/src/resolvers/conversation.ts Show resolved Hide resolved
@hallettj
Copy link
Contributor

hallettj commented Jul 3, 2019

I'm seeing a number of unused variable warnings from eslint. Please make a habit of using the "organize imports" feature in VS Code to remove unused imports, and to put import lines into a consistent order. And if you have not already done so it would be helpful to configure your editor to display eslint warnings.

Copy link
Contributor

@hallettj hallettj left a comment

Choose a reason for hiding this comment

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

There are some remaining things that I would like you to change. And it looks like the merge introduced a problem. When you are ready we can talk more about how replyTo should be produced.

packages/main/src/resolvers/conversation.test.ts Outdated Show resolved Hide resolved
packages/main/src/models/conversation.ts Outdated Show resolved Hide resolved
packages/main/src/models/conversation.ts Outdated Show resolved Hide resolved
packages/main/src/resolvers/conversation.test.ts Outdated Show resolved Hide resolved
@breitman breitman merged commit 48c9298 into master Jul 8, 2019
@breitman breitman deleted the BR/ReplyTo branch July 8, 2019 18:52
@hallettj
Copy link
Contributor

hallettj commented Nov 3, 2019

🎉 This PR is included in version 1.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@hallettj
Copy link
Contributor

hallettj commented Nov 4, 2019

🎉 This PR is included in version 1.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants