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

Replacing const RADIUS by turf-helpers/earthRadius in turf-area (#2045) #2047

Closed
wants to merge 6 commits into from

Conversation

joaquim-rosa
Copy link

Please fill in this template.

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Have read How To Contribute.
  • Run npm test at the sub modules where changes have occurred.
  • Run npm run lint to ensure code style at the turf module level.

This aims to replace the hard-coded spherical Earth radius by the one defined in turf-helpers as proposed in #2045

@joaquim-rosa
Copy link
Author

I've regenerated the test output of the package turf-nearest-neighbor-analysis and run prettier (see last two commits).
All the 6 tests pass if I run the tests locally however they fail in CI... I'm not very familiar with this process so is there anyone that can check this? Thanks.

Copy link
Collaborator

@mfedderly mfedderly left a comment

Choose a reason for hiding this comment

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

The change so far looks good. You are getting test failures in the packages/turf-nearest-neighbor-analysis package which depends on @turf/area. Can you update this PR with the changes from running REGEN=true yarn test and yarn prettier --write ./test/out/ in that package. It seems that the output there is just slightly different after changing the radius value.

import { geomReduce } from "@turf/meta";

// Note: change RADIUS => earthRadius
const RADIUS = 6378137;
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like this is the semi-major radius, and @turf/helpers uses the wgs84 average radius.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for your reply and suggestions.
I have just tried the commands REGEN=true yarn test and yarn prettier --write ./test/out/ in the package turf-nearest-neighbor-analysis and the result is the same of my previous commit, and all 6 tests pass.
Previously, I've done exactly the same but I used npm instead of yarn, with equal results.

I've inspected the section related to the tests of turf-nearest-neighbor-analysis in the CI console output log, where the errors are located, and it sees that it fails because the output used in the asserts is still the old one (and not the one I've committed in the directory /test/out).
So, why is the CI not looking to the latest changes I made on this branch?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Either you need to run yarn build in the turf-area package, or you need to commit the updated test output files in the other package.

Copy link
Author

Choose a reason for hiding this comment

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

I've committed the updated test output files in commits 4bd33e6 and 88a0a03
Maybe I'm missing any step?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like you regenerated them and committed them without prettier and then ran prettier and committed that, but if you look in this PR's list of files changed they aren't listed. That makes me think you might've run the test and prettier commands in that package before running build in turf-area.

Copy link
Author

Choose a reason for hiding this comment

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

I've looked into the commits on the turf-nearest-neighbor-analysis package and there is this message:
"This commit does not belong to any branch on this repository" so this is why their changes are not being picked up by the CI. I've tried a few git commands to add the files of that commit to the branch but without success.
Any ideas on how to achieve that?

@joaquim-rosa
Copy link
Author

After multiple attempts the CI is still not seeing the changes I made in the output of the tests of turf-nearest-neighbor-analysis package, so the tests keep on failing... I guess I'm just not using git in the correct way.
I'm kind of stuck here so any help from the contributors would be appreciated.

@mfedderly mfedderly added this to the 7.0.0 milestone Jul 26, 2021
@mfedderly
Copy link
Collaborator

Closing in favor of #2166

@mfedderly mfedderly closed this Aug 3, 2021
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

2 participants