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

adds implementation of Catalog #448

Merged
merged 3 commits into from
Nov 22, 2022
Merged

adds implementation of Catalog #448

merged 3 commits into from
Nov 22, 2022

Conversation

desaikd
Copy link
Contributor

@desaikd desaikd commented Nov 7, 2022

Description of changes:
This PR works on adding implementation of Catalog.

List of changes:

  • adds implementation of SharedSymbolTable
  • adds implementation of Catalog
    • adds trait Catalog, MutableCatalog
    • adds struct SimpleCatalog

Test:

  • adds unit tests for Catalog implementation.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@desaikd desaikd requested a review from zslayton November 7, 2022 18:45
@codecov
Copy link

codecov bot commented Nov 7, 2022

Codecov Report

Merging #448 (6838d3a) into main (b01a603) will increase coverage by 0.00%.
The diff coverage is 88.15%.

@@           Coverage Diff           @@
##             main     #448   +/-   ##
=======================================
  Coverage   83.56%   83.57%           
=======================================
  Files          85       87    +2     
  Lines       16400    16479   +79     
=======================================
+ Hits        13705    13772   +67     
- Misses       2695     2707   +12     
Impacted Files Coverage Δ
src/lib.rs 78.07% <ø> (ø)
src/catalog.rs 86.44% <86.44%> (ø)
src/shared_symbol_table.rs 94.11% <94.11%> (ø)
src/text/non_blocking/raw_text_reader.rs 91.86% <0.00%> (-0.33%) ⬇️
src/reader.rs 66.66% <0.00%> (-0.26%) ⬇️
ion-c-sys/src/string.rs 88.43% <0.00%> (+0.24%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

src/catalog.rs Outdated Show resolved Hide resolved
src/catalog.rs Show resolved Hide resolved
fn get_table(&self, name: &str) -> IonResult<SharedSymbolTable>;
/// Returns the Shared Symbol Table with given table name and version
/// If a table with given name and version doesn't exists then it returns an error
fn get_table_with_version(&self, name: &str, version: usize) -> IonResult<SharedSymbolTable>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need a get_latest_version? (This will depend on whether we allow inexact matches at import time.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

get_table_with_name returns the latest version of the table based on given table name.

src/catalog.rs Outdated Show resolved Hide resolved
src/catalog.rs Outdated

let versions: &BTreeMap<usize, SharedSymbolTable> =
self.tables_by_name.get(name).ok_or_else(|| {
illegal_operation_raw(format!("Symbol table with name: {} does not exist", name))
Copy link
Contributor

Choose a reason for hiding this comment

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

illegal_operation feels off for this since the user isn't doing anything wrong, but I think it's the closest variant we currently have in IonError. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am open to adding a new Error variant in IonError if that makes sense to you. Maybe get_shared_symbol_table_error or catalog_error or shared_symbol_table_error?

src/catalog.rs Outdated Show resolved Hide resolved
src/catalog.rs Outdated Show resolved Hide resolved
src/shared_symbol_table.rs Outdated Show resolved Hide resolved
src/shared_symbol_table.rs Outdated Show resolved Hide resolved
src/shared_symbol_table.rs Outdated Show resolved Hide resolved
@desaikd desaikd merged commit aa33f0f into main Nov 22, 2022
@almann almann deleted the desaikd-catalog branch May 18, 2023 19:07
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.

2 participants