search

pindell-matt / rust_bucket

Simple JSON key-value store implemented in Rust
Other
11 stars 6 forks source link

Add functions to insert/read/delete json directly #26

Closed pindell-matt closed 8 years ago

pindell-matt commented 8 years ago

Pending tests.

Allows for super-simple JSON storage and extraction.

pub fn store_json(table: &str, json: &str) -> Result<()> {
    try!(create_db_dir());

    let db_table = Path::new("./db").join(table);

    if db_table.exists() {
        return Ok(());
    }

    let mut buffer = try!(File::create(db_table));
    try!(buffer.write_all(json.as_bytes()));

    Ok(())
}

pub fn update_json(table: &str, json: &str) -> Result<()> {
    try!(create_db_dir());

    let db_table = Path::new("./db").join(table);

    let mut buffer = try!(File::create(db_table));
    try!(buffer.write_all(json.as_bytes()));

    Ok(())
}

pub fn read_json(table: &str, json: &str) -> Result<()> {
    read_table(table);

    Ok(())
}

pub fn delete_json(table: &str) -> Result <()> {
    let file = Path::new("./db").join(table);
    try!(fs::remove_file(file));

    Ok(())
}
cite-reader commented 8 years ago

I am growing increasingly distressed by the amount of .unwrap() in this codebase.

As a library, bringing down the isolation unit really isn't our call. Fallible operations should return Result types; that's what Rustaceans expect, and there's lots of support (the try! macro, the new question mark sugar, the higher-order methods on Result) for this style.

If you insist on panic-on-expected-error please consider replacing non-test uses of unwrap with expect, so you at least present meaningful error messages when the process inevitably goes down. On Jun 23, 2016 20:30, "Matt Pindell" notifications@github.com wrote:

Pending tests.

pub fn json_find<T: Serialize + Deserialize>(table: &str, id: &str) -> String { let incoming_record: T = find(table, id); let json_record = serde_json::to_string(&incoming_record); json_record.unwrap() } pub fn json_table_records<T: Serialize + Deserialize>(table: &str) -> String { let records: HashMap<String, T> = get_table_records(table); let json_records = serde_json::to_string(&records); json_records.unwrap() } pub fn json_table<T: Serialize + Deserialize>(table: &str) -> String { read_table(table).unwrap() }

You can view, comment on, or merge this pull request online at:

https://github.com/pindell-matt/rust_bucket/pull/26 Commit Summary

  • Add functions to insert/read/delete json directly

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/pindell-matt/rust_bucket/pull/26, or mute the thread https://github.com/notifications/unsubscribe/AEAKe29PNI2jXYd0cifCQgP52hwwa9_Zks5qO09NgaJpZM4I9b4- .

cite-reader commented 8 years ago

Ugh, the version I saw in the email Github sent me is not the same as the version currently on this page. I blame computers. On Jun 23, 2016 20:40, "Alexander Hill" alexander.d.hill.89@gmail.com wrote:

I am growing increasingly distressed by the amount of .unwrap() in this codebase.

As a library, bringing down the isolation unit really isn't our call. Fallible operations should return Result types; that's what Rustaceans expect, and there's lots of support (the try! macro, the new question mark sugar, the higher-order methods on Result) for this style.

If you insist on panic-on-expected-error please consider replacing non-test uses of unwrap with expect, so you at least present meaningful error messages when the process inevitably goes down. On Jun 23, 2016 20:30, "Matt Pindell" notifications@github.com wrote:

Pending tests.

pub fn json_find<T: Serialize + Deserialize>(table: &str, id: &str) -> String { let incoming_record: T = find(table, id); let json_record = serde_json::to_string(&incoming_record); json_record.unwrap() } pub fn json_table_records<T: Serialize + Deserialize>(table: &str) -> String { let records: HashMap<String, T> = get_table_records(table); let json_records = serde_json::to_string(&records); json_records.unwrap() } pub fn json_table<T: Serialize + Deserialize>(table: &str) -> String { read_table(table).unwrap() }

You can view, comment on, or merge this pull request online at:

https://github.com/pindell-matt/rust_bucket/pull/26 Commit Summary

  • Add functions to insert/read/delete json directly

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/pindell-matt/rust_bucket/pull/26, or mute the thread https://github.com/notifications/unsubscribe/AEAKe29PNI2jXYd0cifCQgP52hwwa9_Zks5qO09NgaJpZM4I9b4- .

selfup commented 8 years ago

@cite-reader

We can refactor tomorrow and return more result types.

selfup commented 8 years ago

Decided to return String types for the three json_ functions when there is an Error.

Developers can now check themselves on the server like so:

let jt = json_table::<sc::Coordinates>("test100");

if j != "'failed to get table'" {
   jt
} else {
  //handle error
}
selfup commented 8 years ago

Here are the refactored functions:

pub fn json_find<T: Serialize + Deserialize>(table: &str, id: &str) -> String {
    let incoming_record: T = find(table, id);
    let json_record = serde_json::to_string(&incoming_record);
    json_record.unwrap_or(String::from("\'failed to get table\'"))
}

pub fn json_table_records<T: Serialize + Deserialize>(table: &str) -> String {
    let records: HashMap<String, T> = get_table_records(table);
    let json_records = serde_json::to_string(&records);
    json_records.unwrap_or(String::from("\'failed to get records\'"))
}

pub fn json_table<T: Serialize + Deserialize>(table: &str) -> String {
    read_table(table).unwrap_or(String::from("\'failed to get table\'"))
}

Here is the diff

pindell-matt commented 8 years ago

@cite-reader thanks for pointing that out, I'm still very new to Rust so I need all the feedback I can get!

cite-reader commented 8 years ago

String is also the wrong return type.

The implied contract of something returning String or usize or any other type that doesn't know anything about errors (so basically anything but Option<T> or Result<T, _>) is that it can't fail, outside of things like OOM conditions. (And in current rust that panics anyway. I think.)

Returning some particular string that's actually an error message creates a confusing API, and confusing APIs cause catastrophic errors in production when someone two sprints ago forgot to check for that error message and handle it, because Sales forgot to mention the thing they sold that was due in a week, so the entire engineering department was in crunch, and it compiled SHIP IT.

Ahem. Sorry, I may have flashed back for a second. The point is this isn't a theoretical concern.

The entire point of taking Result from the ML language family is to make it impossible to confuse an error for a success. The possiblity of failure is visible in the type system, and the program won't compile until the developer has made an explicit decision about how to handle it. Flattening everything may be the right call, but as we're a library rather than the application it's not our call to make.

selfup commented 8 years ago

@cite-reader

Thank you for the detailed explanation. However the tests are failing.

One thing you must understand is that we are pretty new to programming in Rust, and programming in general. So, thanks for being patient and fixing bugs, and making us understand that returning a result is the most important part for this lib (or libs in general).

What you say makes sense, but we might not always have the answer. So we try our best.

Once again, thanks for being patient!

selfup commented 8 years ago

Lil spelling mistake in the commit message but essentially calling unwrap at the end of every function in the test to mimic the developer making function calls.

selfup commented 8 years ago

@pindell-matt

All tests are passing, updated Licensing to say Rust or rust instead of Fe or fe.

All functions now return a result thanks to @cite-reader.

I think we have the most important functionality done.

Documentation should be next.

We can go the cargo doc route, and have our codebase become gigantic (mostly with formatted comments and embedded markdown).

Rust Comments and Documentation

OR

We can write our own documentation in a nice Markdown file.

cite-reader commented 8 years ago

Functions and modules should absolutely use rustdoc. There's very clear-cut usability benefits to putting API documentation in the same form as every other crates' API docs.

High-level documentation, the "what is this and what should I use it for" part, is a little more flexible. A "see the README" link in the crate's top-level rustdoc comment might be sufficient, but it's still nice to have that handled by rustdoc itself, assuming it doesn't inflate to the size of a book. In particular if we have example code.

Oh, and my apologies if I've come off as confrontational; I'd somehow convinced myself that you were new to Rust, but not to proramming in general, so I've probably been assuming our experiences are more similar than they actually are.

selfup commented 8 years ago

@cite-reader

Thank you so much for the thoughtful reply. Yea I have about 1 1/2 years at this point in total programming experience.

I am totally down to have example code, as I have done in selfup-rejs but instead using the built in functionality.

Really enjoy the tooling of Rust a lot more than ruby or JavaScript.

I was also thinking of building an example repository that uses a web framework like nickel or iron and show how it could be used in a microservice or a small production application.

Once again, thank you for being patient 👍