Skip to content

Conversation

@xylar
Copy link
Collaborator

@xylar xylar commented Sep 29, 2016

This merge adds an optional flag for supplying an output directory
to split_features.py instead of writing to a directory with the
component name. This can be helpful for debugging individual
features during manipulation.

@xylar
Copy link
Collaborator Author

xylar commented Sep 29, 2016

This should address #56.

@xylar
Copy link
Collaborator Author

xylar commented Sep 29, 2016

Testing

I tested this with

$ ./merge_features.py -d ocean/region/ -t Mediterranean_Basin
$ ./split_features.py -f features.geojson -o test
$ ls test/region
Adriatic_Sea  Balearic_Sea  Mediterranean_Sea_-_Eastern_Basin  Tyrrhenian_Sea
Aegean_Sea    Ionian_Sea    Mediterranean_Sea_-_Western_Basin
Alboran_Sea   Ligurian_Sea  Strait_of_Gibraltar

It seems to work as expected, splitting individual features into the specified directory instead of one based on the component property.

@xylar
Copy link
Collaborator Author

xylar commented Sep 29, 2016

@pwolfram, does this modification have the behavior you wanted? If not, we should discuss further.

@xylar
Copy link
Collaborator Author

xylar commented Sep 29, 2016

Don't merge this until #52 because this branches off of that branch.

@pwolfram
Copy link
Contributor

Rebased, tested, and merged: thanks @xylar!

@pwolfram
Copy link
Contributor

@xylar, the PR didn't automatically close because I did the rebase locally and I rebased, not you. It looks like it won't automatically close this because of the rebase: http://stackoverflow.com/questions/27740369/pull-requests-merged-manually-after-a-rebase-dont-show-as-merged-on-github. I think if you force push this branch https://github.com/pwolfram/geometric_features/tree/update_split_features_rebase onto your branch it will fix the problem and this should automatically close. Sorry about inconvenience.

@xylar
Copy link
Collaborator Author

xylar commented Sep 29, 2016

@pwolfram, no worries. I think it's best to let me do the rebasing (if necessary) for cases like this to prevent this problem. I'll try the force commit you suggest.

@xylar xylar closed this Sep 29, 2016
@xylar xylar force-pushed the update_split_features branch from 4147142 to 430f826 Compare September 29, 2016 15:49
@douglasjacobsen
Copy link
Member

@pwolfram In general, I agree with @xylar. It's best to tell someone else to rebase their stuff than it is to do it for them. It's a lot easier to prevent issues that way.

@xylar
Copy link
Collaborator Author

xylar commented Sep 29, 2016

Yep, that worked.

@xylar xylar deleted the update_split_features branch September 29, 2016 15:50
@douglasjacobsen
Copy link
Member

Also, one small note. Satisfying PR's this way makes it look like they aren't merged, though it does close them. So, from now on, looking at this PR, it looks like it was closed without it being merged. Not a huge deal, but it can be kind of confusing if you come back here to see if it was merged or not.

@pwolfram
Copy link
Contributor

Agreed @douglasjacobsen. I won't rebase anyone's code in the future. I was just trying to save @xylar some time by doing the rebase myself. We could try the reopen and force-push to see if this fixes this issue but I agree, it isn't a big deal.

@xylar
Copy link
Collaborator Author

xylar commented Sep 29, 2016

I agree, not preferable. So hopefully lesson learned.

@xylar
Copy link
Collaborator Author

xylar commented Sep 29, 2016

@pwolfram, I don't think there's really a way to fix this. I did force push and it thinks the PR is closed rather than merged.

@pwolfram
Copy link
Contributor

The lesson has been learned. I will not rebase others' code prior to a merge. Apologies @douglasjacobsen and @xylar for this snafu.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants