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

#696 FIX #699

Merged
merged 3 commits into from
Aug 24, 2017
Merged

#696 FIX #699

merged 3 commits into from
Aug 24, 2017

Conversation

TikhomirovSergey
Copy link
Contributor

@TikhomirovSergey TikhomirovSergey commented Aug 20, 2017

Change list

Types of changes

  • No changes in production code.
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Details

I suppose that it is the last change and we should publish 5.0.0. All the pended pull-requests will be merged and published at 5.1.0

Mykola Mokhnach and others added 2 commits August 19, 2017 19:07
 - changes from the #698
 - selenium-java was updated to 3.5.1
 - org.apache.commons-lang3 was updated to 3.6
 - org.springframework.spring-context was updated to 4.3.10.RELEASE
 - the element genering was improved. #696 FIX
 - some improvements of AppiumDriver#context
build.gradle Outdated
@@ -54,17 +55,17 @@ compileJava {
}

dependencies {
compile('org.seleniumhq.selenium:selenium-java:3.4.0'){
compile('org.seleniumhq.selenium:selenium-java:3.5.1'){
Copy link
Contributor

@mykola-mokhnach mykola-mokhnach Aug 20, 2017

Choose a reason for hiding this comment

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

it would be nice to know whether it is possible to limit the update of major component version, so it would be possible to avoid similar compilation issues in the future. I'm thinking about the same logic, like in package.json for Node JS modules, where one can say "bla: ^1.0" and this will block the component "bla" from major version update (however, minor version updates are allowed)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mykola-mokhnach Ok I will google how the similar problems are resolved.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@TikhomirovSergey @mykola-mokhnach i think it only restricts selenium-java dependency and not its sub modules or transitive dependency.

Copy link
Member

Choose a reason for hiding this comment

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

@TikhomirovSergey We are good to update to 3.5.2 as 3.5.+. I don't see any issues on yesterday's selenium release.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've almost created the same. Although having Selenium version assigned to a single variable looks more elegant to me ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

or even shorter:

ext.seleniumVersion = '3.4.+'

dependencies {
    compile ("org.seleniumhq.selenium:selenium-java:${seleniumVersion}") {
        force = true

        exclude module: 'cglib'
        exclude group: 'com.google.code.gson'
    }
    compile ("org.seleniumhq.selenium:selenium-support:${seleniumVersion}") {
        force = true
    }
    compile ("org.seleniumhq.selenium:selenium-api:${seleniumVersion}") {
        force = true
    }

Copy link
Member

Choose a reason for hiding this comment

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

Awesome Looks good 👍

Copy link
Contributor Author

@TikhomirovSergey TikhomirovSergey Aug 23, 2017

Choose a reason for hiding this comment

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

@mykola-mokhnach @SrinivasanTarget Hi guys. Thank you for the discussion and advices. Today I am going to update the PR

Copy link
Member

Choose a reason for hiding this comment

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

@mykola-mokhnach
Copy link
Contributor

[ant:checkstyle] [ERROR] /home/travis/build/appium/java-client/src/main/java/io/appium/java_client/AppiumDriver.java:215:9: '}' at column 9 should be on the same line as the next part of a multi-block statement (one that directly contains multiple blocks: if/else-if/else, do/while or try/catch/finally). [RightCurly]
:checkstyleMain FAILED

@TikhomirovSergey
Copy link
Contributor Author

@mykola-mokhnach
Yes I know about the check style issue and I will get it fixed ASAP

@TikhomirovSergey
Copy link
Contributor Author

@mykola-mokhnach @SrinivasanTarget I have updated this PR

@mykola-mokhnach
Copy link
Contributor

Yep, now the PR looks much better. @TikhomirovSergey please merge it

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.

5 participants