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.

Sign up for GitHub

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

Jump to bottom

add storage_config_uri #493

Merged
merged 7 commits into from
Oct 28, 2020
Merged

add storage_config_uri #493

merged 7 commits into from
Oct 28, 2020

Conversation

Karsten1987
Copy link
Collaborator

first step towards #437

sometimes I'd wish our code base was less complex, with less test code. I had to change 34 files to introduce one parameter to open() ;-)

I guess I'll leave this PR as is, with the new functionality throwing one use.

@Karsten1987 Karsten1987 marked this pull request as ready for review August 7, 2020 19:22
@Karsten1987
Copy link
Collaborator Author

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@Karsten1987
Copy link
Collaborator Author

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

emersonknapp
emersonknapp reviewed Aug 18, 2020
View reviewed changes
Copy link
Collaborator

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

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

sometimes I'd wish our code base was less complex

I agree - one opportunity to simplify here would be to deduplicate the code in SequentialCompressionWriter by making it inherit from SequentialWriter

Plus some other comments. The complexity of adding this feature suggests to me that we're overdue for a bit of refactoring - it shouldn't be this hard.

@emersonknapp
Copy link
Collaborator

emersonknapp commented Aug 18, 2020 •
edited
Loading

Note - I am taking a pass at deduplicating all the SequentialCompressionReader/Writer code - restructuring to be an optional plugin called by SequentialReader/SequentialWriter, like the Converter code. This is one thing that should make code like this easier to write

See #511

@emersonknapp emersonknapp mentioned this pull request Aug 18, 2020
Closed
Karsten1987 and others added 4 commits August 31, 2020 16:08
@Karsten1987
Copy link
Collaborator Author

@emersonknapp I kind of re-wrote this PR to call open with a full struct of StorageOptions. That had as a consequence that the definition of that struct had to move to rosbag2_storage instead of rosbag2_cpp - hence the tedious changes from rosbag2_cpp::StorageOptions to rosbag2_storage::StorageOptions.

@Karsten1987 Karsten1987 mentioned this pull request Sep 1, 2020
Merged
emersonknapp
emersonknapp approved these changes Sep 3, 2020
View reviewed changes
Copy link
Collaborator

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

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

Makes sense, this LGTM, no structural concerns.

@Karsten1987 Karsten1987 requested a review from a team as a code owner October 19, 2020 21:20
@Karsten1987 Karsten1987 requested review from thomas-moulard and removed request for a team October 19, 2020 21:20
@Karsten1987
Copy link
Collaborator Author

rebased and new run of CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@Karsten1987
add rosbag2_py to metapackage
6770f39 
Signed-off-by: Karsten Knese <[email protected]>
@Karsten1987
Copy link
Collaborator Author

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

emersonknapp
emersonknapp approved these changes Oct 26, 2020
View reviewed changes
Copy link
Collaborator

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

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

👍 will be glad to merge this - it touches more things than I'd usually prefer

@Karsten1987 Karsten1987 merged commit 5464e6e into master Oct 28, 2020
@delete-merged-branch delete-merged-branch bot deleted the storage_config branch October 28, 2020 16:44
@Karsten1987 Karsten1987 mentioned this pull request Oct 28, 2020
Merged
adamdbrw pushed a commit that referenced this pull request Nov 2, 2020
@Karsten1987 @adamdbrw
add storage_config_uri (#493)
a52af79 
* add storage_config_uri

Signed-off-by: Karsten Knese <[email protected]>

* linters and tests

Signed-off-by: Karsten Knese <[email protected]>

* move storage options to rosbag2_storage

Signed-off-by: Karsten Knese <[email protected]>

* use storage options to open storage backends

Signed-off-by: Karsten Knese <[email protected]>

* add rosbag2_py to metapackage

Signed-off-by: Karsten Knese <[email protected]>
adamdbrw pushed a commit that referenced this pull request Nov 2, 2020
@Karsten1987 @adamdbrw
add storage_config_uri (#493)
79f35d5 
* add storage_config_uri

Signed-off-by: Karsten Knese <[email protected]>

* linters and tests

Signed-off-by: Karsten Knese <[email protected]>

* move storage options to rosbag2_storage

Signed-off-by: Karsten Knese <[email protected]>

* use storage options to open storage backends

Signed-off-by: Karsten Knese <[email protected]>

* add rosbag2_py to metapackage

Signed-off-by: Karsten Knese <[email protected]>
jacobperron added a commit that referenced this pull request Nov 18, 2020
@jacobperron
Add back rosbag2_cpp::StorageOptions as deprecated
eaee8b6 
The struct was removed in #493, but in order
to avoid a hard-break for users coming from Foxy I've added it back with a deprecation
warning.

Signed-off-by: Jacob Perron <[email protected]>
@jacobperron jacobperron mentioned this pull request Nov 18, 2020
Merged
emersonknapp pushed a commit that referenced this pull request Nov 30, 2020
@jacobperron
Add back rosbag2_cpp::StorageOptions as deprecated (#563)
5956681 
The struct was removed in #493, but in order
to avoid a hard-break for users coming from Foxy I've added it back with a deprecation
warning.

Signed-off-by: Jacob Perron <[email protected]>
emersonknapp pushed a commit that referenced this pull request Feb 2, 2021
@Karsten1987
add storage_config_uri (#493)
1703ca0 
* add storage_config_uri

Signed-off-by: Karsten Knese <[email protected]>

* linters and tests

Signed-off-by: Karsten Knese <[email protected]>

* move storage options to rosbag2_storage

Signed-off-by: Karsten Knese <[email protected]>

* use storage options to open storage backends

Signed-off-by: Karsten Knese <[email protected]>

* add rosbag2_py to metapackage

Signed-off-by: Karsten Knese <[email protected]>
emersonknapp pushed a commit that referenced this pull request Feb 2, 2021
@jacobperron
Add back rosbag2_cpp::StorageOptions as deprecated (#563)
99f19e6 
The struct was removed in #493, but in order
to avoid a hard-break for users coming from Foxy I've added it back with a deprecation
warning.

Signed-off-by: Jacob Perron <[email protected]>
emersonknapp pushed a commit that referenced this pull request Feb 17, 2021
@Karsten1987
add storage_config_uri (#493)
bf93d7a 
* add storage_config_uri

Signed-off-by: Karsten Knese <[email protected]>

* linters and tests

Signed-off-by: Karsten Knese <[email protected]>

* move storage options to rosbag2_storage

Signed-off-by: Karsten Knese <[email protected]>

* use storage options to open storage backends

Signed-off-by: Karsten Knese <[email protected]>

* add rosbag2_py to metapackage

Signed-off-by: Karsten Knese <[email protected]>
emersonknapp pushed a commit that referenced this pull request Feb 17, 2021
@jacobperron
Add back rosbag2_cpp::StorageOptions as deprecated (#563)
536b27e 
The struct was removed in #493, but in order
to avoid a hard-break for users coming from Foxy I've added it back with a deprecation
warning.

Signed-off-by: Jacob Perron <[email protected]>
@emersonknapp emersonknapp mentioned this pull request Jun 24, 2021
Closed
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emersonknapp emersonknapp emersonknapp approved these changes

@thomas-moulard thomas-moulard Awaiting requested review from thomas-moulard

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Karsten1987 @emersonknapp