Skip to content
This repository has been archived by the owner on Dec 2, 2018. It is now read-only.

feature/smsdb #28

Merged

Conversation

ThinkDigitalSoftware
Copy link
Contributor

No description provided.

@ThinkDigitalSoftware
Copy link
Contributor Author

Includes the fix I mentioned earlier plus some additional code to get the feature working.

@babariviere
Copy link
Owner

Thanks for the pull request, can you please remove assert, log and required?

@ThinkDigitalSoftware
Copy link
Contributor Author

ThinkDigitalSoftware commented Sep 21, 2018 via email

@ThinkDigitalSoftware
Copy link
Contributor Author

ThinkDigitalSoftware commented Sep 21, 2018 via email

@babariviere
Copy link
Owner

Oh okay so you should let asserts but only remove required

@babariviere
Copy link
Owner

babariviere commented Sep 21, 2018

And maybe can you add a description for each assert? So the developer using this would find it more useful for debug.

Example: assert(message.body, "could not insert SMS because there is no body inside it")

@ThinkDigitalSoftware
Copy link
Contributor Author

Right, ok. Explicit is better than implicit.

@babariviere babariviere merged commit 85a3dae into babariviere:feature/smsdb Sep 21, 2018
@babariviere
Copy link
Owner

Thanks you for this pull request 😁

@ThinkDigitalSoftware
Copy link
Contributor Author

Man, thank you for the library! I'll be submitting improvements for a while, hope you don't mind 😄

@babariviere
Copy link
Owner

It's okay to me haha 😀

@ThinkDigitalSoftware
Copy link
Contributor Author

ThinkDigitalSoftware commented Sep 21, 2018

Any way we can chat? Gitter? discord? Facebook? WhatsApp? or do you prefer here? I Can also open up an issue

@ThinkDigitalSoftware
Copy link
Contributor Author

Also, does your example app have write capabilities yet so I can add some tests?

@ThinkDigitalSoftware
Copy link
Contributor Author

@babariviere what do you think about a new named constructor for SmsMessage?
Maybe something like SmsMessage.valid() or something (Name needs to change) to make on that's good for writing to the SmsDb? Cause I'd rather not see assertion failures and there's not an easy way to avoid errors before running. Just a small convenience.

@babariviere
Copy link
Owner

I have Facebook and discord if you want

@babariviere
Copy link
Owner

I don't think so but you can do it if you want

@babariviere
Copy link
Owner

We can also throw an error from insert function

@ThinkDigitalSoftware
Copy link
Contributor Author

You can find me at thinkdigital#6685 on Discord. I don't think the error is a good idea, because it may not show up until it's in production

@ThinkDigitalSoftware
Copy link
Contributor Author

I'm an advocate of showing errors as soon as possible.

@babariviere
Copy link
Owner

But the error can be catched at runtime

@ThinkDigitalSoftware
Copy link
Contributor Author

OK. You got it. Can you add that? I'm not that good with Java anymore. 😛

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

Successfully merging this pull request may close these issues.

None yet

2 participants