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

Change how we send models to the Logger #2

Closed
wants to merge 6 commits into from

Conversation

PotatoPeeler3000
Copy link
Collaborator

  • Change how the models are being sent
  • write a couple tests as proof of concept
  • changed some print statements to make things more clear
    Also learning how to use GitHub with this PR...

…f concept, changed some print statements to make things more clear
@PotatoPeeler3000 PotatoPeeler3000 self-assigned this Feb 28, 2024
@ihmcrobotics ihmcrobotics locked as too heated and limited conversation to collaborators Feb 28, 2024
@ihmcrobotics ihmcrobotics unlocked this conversation Feb 28, 2024
@PotatoPeeler3000
Copy link
Collaborator Author

Tested on hardware, nothing blew up, looks good

Copy link
Member

@SylvainBertrand SylvainBertrand left a comment

Choose a reason for hiding this comment

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

Main thing is that we shouldn't have nadia specific stuff here. I think we simply need to make the code a little more flexible.

@@ -52,8 +60,7 @@ public byte[] getResourceZip()
ByteArrayOutputStream os = new ByteArrayOutputStream();
try
{
Pattern zipExclude = null; //Pattern.compile(".*\\.(?i)(zip)$");
ClassLoaderTools.createZipBundle(os, zipExclude, resourceDirectories);
ResourceLoaderTools.createZipBundle(os, filter, topLevelResourceDirectories);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Pass down the filter here

@PotatoPeeler3000
Copy link
Collaborator Author

I'd also like to squash these commits before its merged, several of these commits are useless. @SylvainBertrand I know you are trying that with SCS2, you cool if I do that here?

Copy link
Member

@SylvainBertrand SylvainBertrand left a comment

Choose a reason for hiding this comment

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

Ok this looks much better!
pirate-pirate-ship

@PotatoPeeler3000 PotatoPeeler3000 added good first issue Good for newcomers new feature New feature or request labels Mar 1, 2024
@PotatoPeeler3000
Copy link
Collaborator Author

Closing pull request with comment, checking out how this feature works...

@ihmcrobotics ihmcrobotics locked as resolved and limited conversation to collaborators Mar 1, 2024
@ihmcrobotics ihmcrobotics unlocked this conversation Mar 1, 2024
@PotatoPeeler3000
Copy link
Collaborator Author

I guess you don't want to close the PR. You want to merge it, oops

@PotatoPeeler3000
Copy link
Collaborator Author

I think because I force pushed, it wouldn't let me merge this PR, interesting, need to play with that next time. The new PR with the squashed commits is here: #4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants