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

Add priority to database persistence #853

Merged
merged 3 commits into from
Oct 30, 2018

Conversation

guperrot
Copy link
Member

@guperrot guperrot commented Oct 30, 2018

Please have a look at our guidelines for contributions and consider the following before you submit the PR:

  • Has CHANGELOG.md been updated?
  • Are tests passing locally?
  • Are the files formatted correctly?
  • Did you add unit tests?
  • Did you test your change with either the sample apps that are included in the repository or with a blank app that uses your change?

Description

Add priority field to persistence.

/**
* An event can be lost due to low bandwidth or disk space constraints.
*/
public static final int PERSISTENCE_NORMAL = 0x01;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should use a bitwise shift like in iOS?

Suggested change
public static final int PERSISTENCE_NORMAL = 0x01;
public static final int PERSISTENCE_NORMAL = 1 << 0;

Copy link
Contributor

@jaeklim jaeklim Oct 30, 2018

Choose a reason for hiding this comment

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

I'd prefer 0x01. This is Java convention so we can keep hexadecimal number here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally it's more readable for me too. Admittedly it's explicitly different than how we have it in iOS but if we're following language conventions then I don't mind.

Copy link
Member Author

Choose a reason for hiding this comment

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

0x01 is more readable indeed.

@@ -191,15 +208,17 @@ public boolean onUpgrade(SQLiteDatabase db, int oldVersion, int newVersion) {
* @param logJ The JSON string for a log.
* @param targetToken target token if the log is common schema.
* @param targetKey project identifier part of the target token in clear text.
* @param priority priority.
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's align with other parameters - first letter case and article

Copy link
Contributor

@jaeklim jaeklim left a comment

Choose a reason for hiding this comment

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

Overall looks good. Just a couple of minor things.

@@ -765,14 +768,26 @@ public void getLogsException() throws PersistenceException, JSONException {
}
}

@NonNull
private ContentValues getContentValues(DatabasePersistence persistence, String group) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move this method to the bottom or top of the file?

/**
* An event can be lost due to low bandwidth or disk space constraints.
*/
public static final int PERSISTENCE_NORMAL = 0x01;
Copy link
Contributor

@jaeklim jaeklim Oct 30, 2018

Choose a reason for hiding this comment

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

I'd prefer 0x01. This is Java convention so we can keep hexadecimal number here.

@jaeklim
Copy link
Contributor

jaeklim commented Oct 30, 2018

We need DEFAULT constant for flag as per spec. If we remove DEFAULT constant from here, it is not a big deal but we should change the spec before making any changes in the code.
microsoft/appcenter-sdk-apple#1193 (comment)

This PR isn't for public API so not a valid comment.

@jaeklim jaeklim merged commit ca051cf into feature/priority Oct 30, 2018
@guperrot guperrot deleted the feature/priority-database branch October 30, 2018 21:14
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.

4 participants