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

Version 2.0.0 #39

Open
wants to merge 37 commits into
base: dev
Choose a base branch
from
Open

Version 2.0.0 #39

wants to merge 37 commits into from

Conversation

cketti
Copy link
Owner

@cketti cketti commented Jan 13, 2017

Feel free to provide feedback.

Fixes #34
Fixes #42
Fixes #48

Copy link
Contributor

@johnjohndoe johnjohndoe left a comment

Choose a reason for hiding this comment

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

In general the current state seems to work. I integrated the library once in my project.

@@ -7,10 +7,14 @@ android {
defaultConfig {
versionName "1.2.2"

minSdkVersion 4
minSdkVersion 14
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a technical reason for raising the minSdkVersion for ckChangeLog-core and ckChangeLog-dialog here? I changed it back to 4 and compiled locally and could run my app without an issue.

README.md Show resolved Hide resolved
throw new IllegalStateException(e);
}

return result;
Copy link
Contributor

Choose a reason for hiding this comment

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

Return a copy here to avoid that the caller mutates the state of the list.

Copy link
Contributor

Choose a reason for hiding this comment

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

@cketti Not sure if GitHub send you a notification for my review. Please confirm.

Copy link
Owner Author

Choose a reason for hiding this comment

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

It shouldn't be necessary to create a defensive copy, since the ownership of the list basically goes to the caller (XmlParser instances don't outlive the parsing process; see XmlParser.parse(…)).

But I guess we could wrap the list using Collections.unmodifiableList() to avoid callers depending on a modifiable list.

@johnjohndoe
Copy link
Contributor

@cketti Any timeline for the release of v2?

cketti added 9 commits May 12, 2018 17:51
This is mostly due to AAPT(2) optimizing files in res/xml/ and there's
no guarantee this won't break our files. Sadly, there has been at least
one bug that affected ckChangeLog. See issue #48.
Showing a dialog at app start is a very obtrusive way of showing the
change log. Also using a WebView isn't great. The intended message of
adding "legacy" is: Stay away from this artifact when writing new code.
@cketti cketti changed the title [Work in progress] Version 2 Version 2.0.0 May 13, 2018
@cketti
Copy link
Owner Author

cketti commented May 13, 2018

I'm hoping to release 2.0.0 in the next couple of days. Last chance to provide feedback.

@johnjohndoe
Copy link
Contributor

Great! I am trying to look into the changes within this week and will provide feedback.

@johnjohndoe
Copy link
Contributor

johnjohndoe commented May 17, 2018

First review

  1. I recommend using lowercase for both artifacts: ckchangelog-legacy-dialog instead of ckChangeLog-legacy-dialog and ckchangelog-core instead of ckChangeLog-core. A typo due to wrong capitalization is actually pretty hard to spot. I believe lowercase dependency names are common practice. Further, the README is not reflecting the current (camelcase) name.
  2. Please consider recommending implementation "de.cketti.library.changelog:ckchangelog-legacy-dialog:2.0.0" instead of compile 'de.cketti.library.changelog:ckchangelog-legacy-dialog:2.0.0' in the README. Changes are: implementation over compile and double quotes over single quotes.
  3. In my project I have to add both artifacts ckChangeLog-legacy-dialog and ckChangeLog-core in order to show a dialog. It seems that the former artifact does not introduce a transitive dependency. If this is on purpose it should be clearly stated in the README. If ckChangeLog-core is missing a ClassNotFoundException: Didn't find class "de.cketti.changelog.ChangeLog" is thrown.
  4. Is there a technical reason to raise the minSdkVersion from 4 to 14? I saw the support-annotations library - I believe it hasn't changed too much over the last versions. As far as I saw you are only using IdRes, RawRes and Nullable.
  5. The com.android.support:support-v4:27.1.1 dependency in the sample-legacy-dialog module introduces a long list of transitive dependencies which are obsolete. You can replace it with com.android.support:support-fragment:27.1.1.
  6. You could provide a migration guide as a wiki page in addition to the README.

@touficbatache
Copy link

Hi! This should be merged. I am trying to compile with build tools version 28.0.3 but it's failing.

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.

3 participants