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

fix issue #485 : MessagePack queries: Exception during parsing InfluxDB version [macOS] #487

Merged
merged 1 commit into from
Aug 18, 2018

Conversation

lxhoan
Copy link
Contributor

@lxhoan lxhoan commented Aug 14, 2018

No description provided.

@lxhoan lxhoan requested review from fmachado and bednar August 14, 2018 06:29
@lxhoan lxhoan closed this Aug 14, 2018
@lxhoan lxhoan reopened this Aug 14, 2018
@bednar
Copy link
Contributor

bednar commented Aug 14, 2018

Maybe it will be better use some implementation for the Semantic Versioning - semver4j.

The method checkMessagePackSupport will be pretty simple:

private boolean checkMessagePackSupport() {
    Semver version = new Semver(version(), Semver.SemverType.NPM);
    return version.isGreaterThan("1.4");
}

@codecov-io
Copy link

codecov-io commented Aug 14, 2018

Codecov Report

Merging #487 into master will increase coverage by 0.05%.
The diff coverage is 91.66%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #487      +/-   ##
============================================
+ Coverage     87.29%   87.35%   +0.05%     
- Complexity      357      362       +5     
============================================
  Files            24       24              
  Lines          1496     1503       +7     
  Branches        164      167       +3     
============================================
+ Hits           1306     1313       +7     
  Misses          124      124              
  Partials         66       66
Impacted Files Coverage Δ Complexity Δ
src/main/java/org/influxdb/impl/InfluxDBImpl.java 89.78% <91.66%> (+0.19%) 78 <3> (+5) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update becbfb5...f134e5b. Read the comment docs.

@lxhoan
Copy link
Contributor Author

lxhoan commented Aug 14, 2018

@bednar
I don't think main project maintainers will approve for new dependencies (semver4j) if it not indispensable

Copy link
Contributor

@bednar bednar left a comment

Choose a reason for hiding this comment

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

@lxhoan I tried the changes on macOS and now the MessagePack queries works

@lxhoan
Copy link
Contributor Author

lxhoan commented Aug 14, 2018

there unit test added for the new version checking, it should cover MacOS case and even unrealistic cases

@lxhoan
Copy link
Contributor Author

lxhoan commented Aug 15, 2018

@fmachado , please have a look on it, tks

@majst01 majst01 merged commit f31b2fd into influxdata:master Aug 18, 2018
@fmachado
Copy link
Contributor

@lxhoan I know this PR is already merged (and apologies for the delay) but as you requested, I'll add my comments.

@@ -666,13 +669,26 @@ public boolean databaseExists(final String name) {
public String error;
}

private boolean checkMessagePackSupport() {
Matcher matcher = Pattern.compile("(\\d+\\.*)+").matcher(version());
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK, Pattern.compile creates an immutable object so you can reuse it. There is another regular expression that would help you so you don't have to split: (?:(\d+)\.)(\d+). (you can see it working here). You still have to convert it to int, though.

@lxhoan lxhoan added this to the 2.13 milestone Sep 7, 2018
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

5 participants