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

Attach meta data to message #50

Closed
wants to merge 6 commits into from
Closed

Attach meta data to message #50

wants to merge 6 commits into from

Conversation

sualko
Copy link
Contributor

@sualko sualko commented Mar 18, 2014

According to #49

@arlolra
Copy link
Owner

arlolra commented Mar 18, 2014

This is looking alright. Could you squash the commits and remove any changes to the build directory. I guess you're ok with fragmented messages always returning the same meta data.

Also, you seem to be missing the path where messaged get stored for later delivery. I think that needs mending.

@arlolra
Copy link
Owner

arlolra commented Mar 18, 2014

Adding a test would be nice.

@sualko
Copy link
Contributor Author

sualko commented Mar 19, 2014

  • squash commits
  • remove changes from the build directory
  • consider stored messages
  • write some tests
  • update the documentation

(I've always wanted to use this task list :-))

@sualko
Copy link
Contributor Author

sualko commented Mar 19, 2014

How is the way to run your unit tests? Maybe you can give me short explanation.

@arlolra
Copy link
Owner

arlolra commented Mar 19, 2014

make test

The test harness is mocha, which should be available when you npm install. The libotr test will fail unless you cd test; make as well. There's also Travis CI integration so if you enable that on your fork it'll lint the code and run the tests.

if ( this.REQUIRE_ENCRYPTION ||
this.msgstate !== CONST.MSGSTATE_PLAINTEXT
) {
msg = CryptoJS.enc.Utf8.parse(msg)
msg = msg.toString(CryptoJS.enc.Latin1)
}
this._sendMsg(msg)
this._sendMsg(msg, null, meta)
Copy link
Owner

Choose a reason for hiding this comment

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

I think false is better than null here.

@arlolra
Copy link
Owner

arlolra commented Mar 19, 2014

A final thing to add to your list: please update the documentation.

Thanks for all the good work.

@sualko
Copy link
Contributor Author

sualko commented Mar 20, 2014

Now I hope everything is satisfying. I also added my owncloud and sogo app to the "in the wild" section.

@arlolra
Copy link
Owner

arlolra commented Mar 22, 2014

Merged manually (needed a rebase).

Thanks @sualko!

@arlolra arlolra closed this Mar 22, 2014
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

2 participants