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

#1285 - fixing EOL to be platform specific #1462

Closed
wants to merge 1 commit into from

Conversation

fejese
Copy link
Contributor

@fejese fejese commented Feb 25, 2017

Fix for #1285

Any of the checked boxes below indicate that I took action:

For all non-trivial changes that modify the behavior or public API:

  • Before submitting a pull request, I started a discussion on the Gradle developer list,
    the forum or can reference a JIRA issue.
  • I considered writing a design document. A design document can be
    brief but explains the use case or problem you are trying to solve,
    touches on the planned implementation approach as well as the test cases
    that verify the behavior. Optimally, design documents should be submitted
    as a separate pull request. Samples
    can be found in the Gradle GitHub repository. Please let us know if you need help with
    creating the design document. We are happy to help!
  • The pull request contains an appropriate level of unit and integration
    test coverage to verify the behavior. Before submitting the pull request
    I ran a build on my local machine via the command
    ./gradlew quickCheck <impacted-subproject>:check.
  • The pull request updates the Gradle documentation like user guide,
    DSL reference and Javadocs where applicable.

@bmuschko
Copy link
Contributor

Thanks for providing the pull request. Before we can accept the change we'd like to see an integration test that verifies the correct behavior on Linux and Windows. Could you please provide one?

@fejese
Copy link
Contributor Author

fejese commented Feb 28, 2017

@bmuschko , of course, makes sense. One question though: do you run the tests both on windows and linux (so I could make use of %s in one test) or do you have a usual way of injecting system properties in tests (I guess by setting the line.separator system property would be the most correct way of testing this)?

@eriwen
Copy link
Contributor

eriwen commented Mar 1, 2017

@fejese Yes, we run tests on Windows and Linux. You should be able to find other integration test examples with properties set in the code base. Let us know if you have troubles.

@fejese
Copy link
Contributor Author

fejese commented Mar 3, 2017

@eriwen Thanks for the help. I've looked an it seems that setting the system property for the test is not working as this property is cached already in System.lineSeparator(). So I've updated the test to always check for platform specific line ending. What do you think? The PR is rebased and updated.

@lacasseio
Copy link
Contributor

Thanks @fejese for this issue. Thanks to your sharp laser eye, we can use this PR to fix issue #1546 introduced by the new console. I will see to merge this before 3.5 RC1

@lacasseio lacasseio added this to the 3.5 RC1 milestone Mar 10, 2017
@lacasseio lacasseio self-assigned this Mar 10, 2017
@lacasseio lacasseio self-requested a review March 10, 2017 18:24
@lacasseio
Copy link
Contributor

I rebased this PR on release and manually merge the PR. I will close this.

@lacasseio lacasseio closed this Mar 10, 2017
@ov7a ov7a removed this from the 3.5 RC1 milestone Mar 28, 2024
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