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

GTDKeyFormatter support #138

Merged
merged 5 commits into from
Apr 8, 2021
Merged

GTDKeyFormatter support #138

merged 5 commits into from
Apr 8, 2021

Conversation

gchenfc
Copy link
Member

@gchenfc gchenfc commented Apr 7, 2021

This provides support for GTDKeyFormatter. Fixes #131

To be useful, this requires borglab/wrap#81 to be merged. Otherwise, GTDKeyFormatter is exposed but can't be passed into print_ since print_ is broken currently.

Copy link
Collaborator

@yetongumich yetongumich left a comment

Choose a reason for hiding this comment

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

LGTM! This improvement is very useful!

@varunagrawal
Copy link
Collaborator

@gchenfc came up with a much easier way of doing this (as well as for supporting global variables) so we can close this.

@gchenfc
Copy link
Member Author

gchenfc commented Apr 8, 2021

Yes, but it's going to require a couple additional PR's and also the calling won't be quite as transparent because you will have to manually call fg.print_("", gtd.GTDKeyFormatter) every time instead of using the GTD key formatter by default.

I would propose still merging this in so that people can start using it right away, and then also adding in the "better" version as PR's land.

@gchenfc gchenfc merged commit 5126a1b into master Apr 8, 2021
@gchenfc gchenfc deleted the feature/keyformatprint branch April 8, 2021 08:43
@varunagrawal
Copy link
Collaborator

Immediate use is not a reason to rush a PR, especially since I was the one who raised the issue initially. :)

That being said, some parts of this are missing documentation explaining the rationale behind them. If we're adding convenient hacks they need to be documented so that they are not mysterious in the future.

@varunagrawal
Copy link
Collaborator

Hold up, why was this merged if #136 was merged? Aren't they the same thing???

@varunagrawal
Copy link
Collaborator

@gchenfc you have to please stop making multiple PRs for similar things. This is making reviewing difficult and unfortunately you got trigger happy with this one, merging it too soon. My guess is this one relies on the now closed PR for wrap which we decided against in favor of supporting global variables?

@dellaert
Copy link
Member

dellaert commented Apr 8, 2021

@gchenfc you have to please stop making multiple PRs for similar things. This is making reviewing difficult and unfortunately you got trigger happy with this one, merging it too soon. My guess is this one relies on the now closed PR for wrap which we decided against in favor of supporting global variables?

Try rephrasing this in a more respectful way :-)

@varunagrawal
Copy link
Collaborator

I'm sorry about that. It wasn't my intention to come across as disrespectful.

@gchenfc
Copy link
Member Author

gchenfc commented Apr 9, 2021

damn sorry guys, I got confused by the different repos and branches and also I think the merge targets were a little bit funky. Combined with the fact that I was really eager to be able to use this code because I was spending hours debugging unit tests by matching up 15 digit integers by eye, I did get too trigger happy. I probably should have just merged this branch in with my cable robot branch, but I was worried that if hypothetically this ended up not getting merged then the divergence would be challenging to deal with. Curious in general what the git protocol is if I need changes from branch A in branch B before branch A has merged with master/develop.

I'm having trouble remembering now, but I think #136 added the "hacky" python multiple-inheritance thing, whereas this PR just exposed the global variable GTDKeyFormatter. I think the diff just looks weird because the merge target should have been the #136 branch, and you can see that the first commit of this branch is actually the same as #136's branch. So to summarize, this PR is indeed adding a new feature completely distinct from #136, but the diff just looks weird because I didn't set the targets properly.

I also totally missed Frank's request in #136 to add documentation so I'll make a "final" PR to clean all this stuff up and add documentation. My bad here.

TL;DR My bad, i'm going to go make a "clean-up" PR

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.

GTDKeyformatter is not working
4 participants