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

Nordic to use preferred origin and handle large negative longitudes #2195

Merged
merged 1 commit into from Jul 5, 2018

Conversation

Projects
None yet
3 participants
@calum-chamberlain
Contributor

calum-chamberlain commented Jul 5, 2018

What does this PR do?

Forces nordic IO to write the preferred origin rather than the first one. Also properly fixes for large negative longitude values (<= -100).

Selected master for this as #2181 was tagged at 1.2.0.

Why was it initiated? Any relevant Issues?

Closes #2181, and properly fixes #1991 (which was patched so that reading happened, but writing was improperly formatted, generating files that failed the test for _is_sfile).

PR Checklist

  • Correct base branch selected? master for new features, maintenance_... for bug fixes
  • This PR is not directly related to an existing issue (which has no PR yet).
    - [ ] If the PR is making changes to documentation, docs pages can be built automatically.
    Just remove the space in the following string after the + sign: "+ DOCS"

    - [ ] If any network modules should be tested for the PR, add them as a comma separated list
    (e.g. clients.fdsn,clients.arclink) after the colon in the following magic string: "+TESTS:"
    (you can also add "ALL" to just simply run all tests across all modules)
  • All tests still pass.
  • Any new features or fixed regressions are be covered via new tests.
  • Any new or changed features have are fully documented.
  • Significant changes have been added to CHANGELOG.txt .
    - [ ] First time contributors have added your name to CONTRIBUTORS.txt .
@calum-chamberlain

This comment has been minimized.

Contributor

calum-chamberlain commented Jul 5, 2018

I think that the appveyor test fails are unrelated, and it looks like the nordic tests still pass and have more tests.

@krischer

This comment has been minimized.

Member

krischer commented Jul 5, 2018

I think that the appveyor test fails are unrelated, and it looks like the nordic tests still pass and have more tests.

Yea - they are "fixed" in #2188.

@krischer

👍

I'm not an expert in the Nordic format but the changes look solid to me and are tested.

@krischer krischer merged commit 9a723e6 into master Jul 5, 2018

2 of 5 checks passed

continuous-integration/appveyor/branch AppVeyor build failed
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
docker-testbot docker testbot results not available yet
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@calum-chamberlain calum-chamberlain deleted the Nordic_preferred branch Jul 5, 2018

@megies

This comment has been minimized.

Member

megies commented Jul 9, 2018

The longitude fix couldve gone into maintenance, but I guess we just leave as is. I'll add a changelog in any case.

megies added a commit that referenced this pull request Jul 9, 2018

@calum-chamberlain

This comment has been minimized.

Contributor

calum-chamberlain commented Jul 9, 2018

Yeah, sorry - they are really small changes and I could just make the same changes on maintenance if you want? Just the changes at lines 1154 and 1159, and the test here.

@megies

This comment has been minimized.

Member

megies commented Jul 9, 2018

Yeah, sorry - they are really small changes and I could just make the same changes on maintenance if you want? Just the changes at lines 1154 and 1159, and the test here.

No worries, whatever you think is appropriate.

@calum-chamberlain

This comment has been minimized.

Contributor

calum-chamberlain commented Jul 9, 2018

I think that depends on the release plan - I'm not keen on leaving the issue with longitudes < -100 in for the next release...

@megies

This comment has been minimized.

Member

megies commented Jul 9, 2018

Well, I could state some assumptions on release plan.. but so far we never managed to stick to any release plans, so.. ;-)

I'm not keen on leaving the issue with longitudes < -100 in for the next release...

Not sure I understand what you're saying.. you want to fix it for 1.1.1 (which is much more foreseeable than 1.2.0)? If that's the case, feel free to make a PR against maintenance branch for the longitude stuff.

@calum-chamberlain

This comment has been minimized.

Contributor

calum-chamberlain commented Jul 9, 2018

Yeah, sorry, I will make a PR for the longitude stuff.

@calum-chamberlain calum-chamberlain referenced this pull request Jul 9, 2018

Merged

Read and write large negative longitudes #2197

5 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment