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

Coverage improvement. #462

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Coverage improvement. #462

wants to merge 5 commits into from

Conversation

nursultanturdaliev
Copy link
Contributor

Code coverage has been improved by 4% overall.

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @RoyMontoya, @whoshuu and @jivimberg to be potential reviewers

@sibelius
Copy link
Contributor

One thing that should be really tested is the creation and migration of the database

@nursultanturdaliev
Copy link
Contributor Author

Ok I will test them too then :)

@nursultanturdaliev
Copy link
Contributor Author

@sibeliusseraphini On last commit I renamed SugarContext.getSugarContext() to SugarContext.getInstance() because it returns private instance property if not null.
If the private property is instance then calling it's getter method getInstance if more convenient, in my opinion. How do you think?

@sibelius
Copy link
Contributor

sibelius commented Feb 3, 2016

@nurolopher the only problem of changing a public API name is that you could introduce a breaking change to another person codes.

However, in this case, I think this method is used almost exclusively by Sugar implementation, so no problem.

Thanks, I will merge it this week

@nursultanturdaliev
Copy link
Contributor Author

👍

@JonatanSalas
Copy link
Collaborator

@nurolopher could you check the conflicts?

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

Successfully merging this pull request may close these issues.

4 participants