[WIP] feature_database #8

Open
dkanus wants to merge 2 commits from feature_database into master
dkanus commented 4 years ago
Owner

This implements a a simple JSON-based database, that works through loading full self-copy into the memory. Should be more than enough for the needs of kf modding.

Closes #6
Closes #1

This implements a a simple JSON-based database, that works through loading full self-copy into the memory. Should be more than enough for the needs of kf modding. Closes #6 Closes #1
dkanus requested review from Ggg_123 4 years ago
dkanus added the
enhancement
label 4 years ago
dkanus added this to the alpha1 milestone 4 years ago
dkanus requested review from Chaos 4 years ago
Chaos reviewed 4 years ago
Chaos left a comment
Collaborator

Test comment

Test comment
Chaos reviewed 4 years ago
Chaos left a comment
Collaborator

Sorry for spam

Sorry for spam
Chaos reviewed 4 years ago
Chaos refused to review 4 years ago
Ggg_123 requested changes 4 years ago
custom_error! { pub IncorrectPointer{pointer: String} = "Incorrect pointer is specified: {pointer}" }
/// This is a enum that used internally to refer to values inside of
Collaborator

Hard to read/understand. Possible improvements:
aan enum?
is used internally?
inside JSON objects/arrays, using serde?
inside of JSON object?
What is a "failed state"?

Hard to read/understand. Possible improvements: ~~a~~an enum? *is* used internally? inside JSON objects/arrays, using serde? inside ~~of~~ JSON object? What is a "failed state"?
Invalid,
}
/// Implements database's file by wrapping JSON value (`serde_json::Value`)
Collaborator

"Stores database in a JSON file"?

"Stores database in a JSON file"?
}
impl File {
/// Creates an empty file that will contain an empty JSON object.
Collaborator

Empty file cannot contain anything.

Empty file cannot contain anything.
}
}
/// Loads JSON value from the specified file.
Collaborator

But it accepts file_contents, not file/filepath?

But it accepts file_contents, not file/filepath?
})
}
/// Returns file's "root", - JSON value contained inside it.
Collaborator

,-? Hard to understand.

,-? Hard to understand.
&self.contents
}
/// Attempts to return JSON value, corresponding to the given JSON pointer.
Collaborator

Remove "attempts"?

Remove "attempts"?
/// 1. If it already exists, - it will be overwritten.
/// 2. If it does not exist, but it's parent object/array does -
/// it will be added.
/// 3. Otherwise an error will be raise.
Collaborator

raised

raised
/// it will be added.
/// 3. Otherwise an error will be raise.
///
/// If array needs to be expanded, - missing values will be filled
Collaborator

,-?

,-?
/// 3. Otherwise an error will be raise.
///
/// If array needs to be expanded, - missing values will be filled
/// with `json!(null)`, i.e. inserting `7` at index `5` in array `[1, 2, 3]`
Collaborator

Sounds like a hack/bug - why is it like this?

Sounds like a hack/bug - why is it like this?
Ok(())
}
/// Removes (and returns) value specified by theJSON pointer.
Collaborator

Missing space

Missing space
}
/// Removes (and returns) value specified by theJSON pointer.
/// If it did not exist - returns `None`.
Collaborator

Remove 2nd line? A simple "if it exists" should be enough.

Remove 2nd line? A simple "if it exists" should be enough.
}
}
/// Helper method to create a value if missing (as `json!(null)`).
Collaborator

What's missing?

What's missing?
ValueReference::Array(vec, variable_index) => {
// We've checked at the beginning of this method that value
// at `variable_index` does not exist, which guarantees
// that array is to short and we won't shrink it
Collaborator

"too short"
Also, the " We've checked ... we won't shrink it" part is hard to understand - checked what where? Why can't we shrink it? How are those 2 things connected?

"too short" Also, the " We've checked ... we won't shrink it" part is hard to understand - checked what where? Why can't we shrink it? How are those 2 things connected?
Ok(())
}
/// Helper method, - converts JSON pointer into auxiliary `ValueReference` enum.
Collaborator

,-?

,-?
if pointer.is_empty() {
return ValueReference::Invalid;
}
// Extract variable name (that `pointer` points to)
Collaborator

Need a link to https://tools.ietf.org/html/rfc6901 somewhere to explain what exactly "JSON pointer" is.

Need a link to https://tools.ietf.org/html/rfc6901 somewhere to explain what exactly "JSON pointer" is.
Some(v) => v,
_ => return ValueReference::Invalid,
};
// For arrays we also need to confirm validity of the variable name
Collaborator

"validity of the variable name" - which means?..

"validity of the variable name" - which means?..
}
}
// Helper function to disassemble JSON path.
Collaborator

Into what?

Into what?
Ok(())
}
/// helper function to read a group from a given subdirectory:
Collaborator

helper->Helper
Containing what?

helper->Helper Containing what?
fn get_file_name(path: &path::Path) -> String {
path.file_stem()
.and_then(|x| x.to_str())
.unwrap_or_default()
Collaborator

Wont this silently cause errors by replacing given path that cannot be converted with empty strings?
Same thing in fn below this one.

Wont this silently cause errors by replacing given path that cannot be converted with empty strings? Same thing in fn below this one.
/// This database is only supposed to hold a relatively small amount of data
/// that:
///
/// 1. can be freely and full loaded into memory;
Collaborator

full -> fully

full -> fully
/// Directory must contain valid database and be readable.
///
/// This method will also remove all data from the current database's path
/// and fail if it can't.
Collaborator

fail -> will fail

fail -> will fail
}
/// Creates a new empty group.
/// Will produce error if group already exists.
Collaborator

Produce -> return

Produce -> return
Ok(())
}
/// Checks if specified file (in a specified group) is contained in the database.
Collaborator

contained -> stored

contained -> stored
.expect("Missing value that was just inserted."))
}
/// Removes specified file (in a specified group).
Collaborator

Braces aren't needed here.

Braces aren't needed here.
}
/// Removes specified file (in a specified group).
/// Will produce error if file does not exist.
Collaborator

produce -> return

produce -> return
Ok(())
}
/// Returns immutable reference to the specified file (in a specified group) as `file::File`.
Collaborator

Braces aren't needed here.

Braces aren't needed here.
}
}
// Helper methods that return (im)mutable reference to `HashMap`
Collaborator

(im)? Why?

(im)? Why?
impl Drop for TestCleanup {
fn drop(&mut self) {
let _ = fs::remove_dir_all(&self.path);
Collaborator

Why _?

Why _?
_ => (),
}
let db = database::Database::load(Path::new(filename));
/*match db {
Collaborator

Remove commented code?

Remove commented code?
Ggg_123 reviewed 4 years ago
/// Implements database's file by wrapping JSON value (`serde_json::Value`)
/// and providing several convenient accessor methods.
#[derive(Debug)]
pub struct File {
Collaborator

Rename to JsonFile?
Also, it's not even a file.

Rename to JsonFile? Also, it's not even a file.
Ggg_123 reviewed 4 years ago
let group_index = self.group_index(group_name)?;
Ok(&mut (&mut self.groups[group_index]).files)
}
Collaborator

Missing doc.

Missing doc.
dkanus changed title from feature_database to [WIP] feature_database 3 years ago

Reviewers

Ggg_123 requested changes 4 years ago
This pull request is marked as a work in progress.
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.