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

Issue 9275 - GeoJsonDataSource process method #9276

Merged
merged 14 commits into from
Apr 12, 2022

Conversation

thw0rted
Copy link
Contributor

@cesium-concierge
Copy link

Thanks for the pull request @thw0rted!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

@thw0rted
Copy link
Contributor Author

About half of this PR is fixes for unspecified or inconsistent JSDoc -- if I need to break that out separately I can, but since I was already touching the code in question I figured it wouldn't hurt.

@cesium-concierge
Copy link

Thanks again for your contribution @thw0rted!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@hpinkos
Copy link
Contributor

hpinkos commented Jan 11, 2021

Thanks for the PR @thw0rted. Sorry we haven't had a chance to review this yet. I skimmed through the changes and they look fine. I'll try to make time to give this a thorough review soon.

@cesium-concierge
Copy link

Thanks again for your contribution @thw0rted!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@cesium-concierge
Copy link

Thanks again for your contribution @thw0rted!

No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

2 similar comments
@cesium-concierge
Copy link

Thanks again for your contribution @thw0rted!

No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@cesium-concierge
Copy link

Thanks again for your contribution @thw0rted!

No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@sanjeetsuhag sanjeetsuhag self-assigned this Feb 3, 2022
Copy link
Contributor

@sanjeetsuhag sanjeetsuhag left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @thw0rted. This PR looks mostly in good shape. A few minor changes will get it over the finish line.

@thw0rted
Copy link
Contributor Author

thw0rted commented Feb 4, 2022

Oh wow, I totally forgot about this -- I'll have to dig through my notes and try to remember why I needed this 😁

I hope it's OK that I rebased with force-push rather than a merge commit, that's how I usually work locally.

Re: vscode files, I noticed that gitignore already has entries for WebStorm, would it make sense to add one for .vscode/ before closing this out?

Also also: I have no idea how out of date #8903 is but while I have your attention, maybe somebody could take a look?

@ggetz
Copy link
Contributor

ggetz commented Feb 7, 2022

I have no idea how out of date #8903 is but while I have your attention, maybe somebody could take a look?

@thw0rted We're a bit bottled-necked in terms of devs with enough typescript knowledge, but we'll try to get to it soon. Thanks for your patience!

Copy link
Contributor

@sanjeetsuhag sanjeetsuhag left a comment

Choose a reason for hiding this comment

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

Just revisiting this PR, seems like it's close to a merge, but needs an update pass. Also, there are some unrelated changes here in the form of TypeScript changes but it would be preferred if they can be put in a dedicated PR like #8903. The gulpfile.cjs change is probably ok.

@thw0rted thw0rted force-pushed the issue-9275 branch 2 times, most recently from 16c9a02 to 9c75ae1 Compare April 12, 2022 09:51
@thw0rted
Copy link
Contributor Author

I've merged the latest from main and I think addressed all the comments above. Please let me know if anything else needs to be changed, or feel free to make the fix yourself, whatever is easiest.

@thw0rted
Copy link
Contributor Author

FYI I went ahead and updated #8903 as well, since you're looking at this. There are a bunch of what turned out to be trivial fixes due to the when.js deprecation. I fixed @returns {Promise<void>} to @returns {when.Promise.<void>}, since the dotted-generic notation is technically correct in JSDoc, then when when was removed I replaced when.Promise with Promise, so you're left with a bunch of file changes that add a single .. If there's an easy way to remove all those changes from the PR, let me know, I'm still looking for it.

@thw0rted
Copy link
Contributor Author

I'm confused, it looks like main has a .vscode/tasks.json and your commit just deleted that file. Was this intentional?

@sanjeetsuhag
Copy link
Contributor

@thw0rted Good catch. I deleted it because in main, the file is in .vscode/tasks.json. The file in this PR was in the root directory.

image

But now, looking at the PR diff, it's deleting it inside .vscode/tasks.json...

Copy link
Contributor

@sanjeetsuhag sanjeetsuhag left a comment

Choose a reason for hiding this comment

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

Using this Sandcastle, I confirmed that this was working as expected with two GeoJSONs loading, first with load, then with process.

image

Thanks again @thw0rted!

@sanjeetsuhag sanjeetsuhag merged commit cad8ca3 into CesiumGS:main Apr 12, 2022
@thw0rted thw0rted deleted the issue-9275 branch April 13, 2022 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Issue/PR closed
Development

Successfully merging this pull request may close these issues.

5 participants