leggettc18 / fundwarrior Goto Github PK

In GitLab by @leggettc18 on Sep 28, 2018, 23:43

Merges duplicate-name-fix -> master

Putting funds with duplicate names into the fund list resulted in the new fund being inaccessible, since get_by_name would find the old one first and return that. Fund::new() did not provide a way to check if the Fund was already in the list, and letting it take the fund list as an argument felt overcomplicated. Instead, I opted to create a FundManager struct. This is a very simple struct. At the moment, it only contains the fund list, as well as functions that previously operated on that list. For example, get_by_name() is now get_fund_by_name and it is a method of the FundManager struct rather than the Fund struct. I also added a add_fund method of FundManager. This is basically just a wrapper for Fund::new() but since FundManager contains the fund list, it can check it for duplicate names and returns an error if it finds one.

In GitLab by @leggettc18 on Oct 2, 2018, 15:11

Nothing is well-documented right now. I need to add rust documentation comments to public functions at a minimum

In GitLab by @leggettc18 on Oct 1, 2018, 20:04

Merges display -> master

• Implements Display trait on Fund with a different output format.
• Changes the printing functions of FundManager to make use of Fund's Display trait.
• Changes the list command to info
• Adds a print_fund instruction to the end of the new, spend, and deposit command to display the change the user just made

Closes #5

In GitLab by @leggettc18 on Oct 5, 2018, 19:13

While adding the documentation comments I noticed a few ways I could simplify the file Paths. I believe I can even use regular Paths instead of PathBufs which is probably safer and removes some more unnecessary cloning.

In GitLab by @leggettc18 on Oct 7, 2018, 18:27

In GitLab by @leggettc18 on Oct 2, 2018, 21:50

Merges transfer -> master

Transferring between funds works with these additions. I was not able to code it the way I wanted. I wanted to code a transfer function, but since it required borrowing self as mutable, I couldn't call get_fund_by_name twice in the same method. I was, however able to do it by simply doing the spend and deposit steps in the match arm for the transfer command.

Closes #10

In GitLab by @leggettc18 on Oct 2, 2018, 13:16

Merges arg-parse -> master

Found the correct way to simplify the argument parsing stage thanks to some help from /u/usernamedottxt on Reddit. Closes #2

In GitLab by @leggettc18 on Oct 8, 2018, 16:30

Merges editing-features -> master

This merge requests adds functionality for renaming funds and updating the values of amount and goal of a Fund to a specific amount, rather than spending or depositing certain amounts. Also adds necessary methods, documentation, and tests, as well as a field value on the Config struct, and the necessary argument parsing for it.

Closes #19

In GitLab by @leggettc18 on Sep 28, 2018, 05:48

Merges money-as-pennies -> master

Makes necessary changes for the program to store integers instead of floating point numbers, adds a function to display this integer as a dollar amount with a decimal point, and changes the info display functions accordingly.

Closes #3

In GitLab by @leggettc18 on Oct 12, 2018, 18:27

Merges alt-fundfile -> master

This MR replaces the -c option (which is currently useless) with the -f flag, which allows the FundWarrior program to accept an alternate location of a fundfile. If one is not specified, the program still uses the default location of data_dir/fund/fund, where data_dir is the path provided by the data_dir function of the dirs crate, the first fund is a directory within that, and the second fund is the actual fundfile.

In GitLab by @leggettc18 on Oct 7, 2018, 18:33

In GitLab by @leggettc18 on Sep 27, 2018, 20:30

Merges load-fix -> master

This is an attempt to fix the load function so it returns an error instead of panicking upon a parser error. This could still be improved by returning more descriptive error messages to help a user potentially find the error.

Example: If for whatever reason, the period of either the amount or goal stored in the fund file were replaced with a comma, we get the following error message: Application error invalid float literal. Instead, it may be better to have an error message like the following: Application error: while parsing ~/.fund: invalid float literal. This way the user knows which file the error is in, and may be able to go and fix it. It is also worth considering this type of error message in the future when config file parsing starts getting implemented.

Closes #6

In GitLab by @leggettc18 on Sep 27, 2018, 18:18

/u/usernamedottxt on Reddit said

TYPICALLY, instead of a 'print_all' function, you'd wrap the type and implement std::fmt::Display. It's fine here, just a small thing you could change later if you wanted to

https://www.reddit.com/r/rust/comments/9jdjpc/my_new_project_in_rust_open_to_critiques_and/e6qno3f

In GitLab by @leggettc18 on Oct 5, 2018, 19:49

Merges path-refactor -> master

I was able to simplify the path handling slightly. At the very least I was able to remove the to_owned calls in the save and load functions that were causing unnecessary cloning. I also updated the documentation accordingly and removed an unnecessary import of PathBuf.

In GitLab by @leggettc18 on Sep 27, 2018, 18:16

https://www.reddit.com/r/rust/comments/9jdjpc/my_new_project_in_rust_open_to_critiques_and/e6qno3f

In GitLab by @leggettc18 on Oct 7, 2018, 18:33

In GitLab by @leggettc18 on Oct 10, 2018, 18:05

Merges remove-printing -> master

Closes #22
These printing functions do not need to be in the library crate. All of the same information can be obtained via getters, iterators the Display trait, and the display_dollars helper function. This MR removes them from the library crate, and adds them to the binary crate, refactoring it so that the old methods are removed while still printing all the same information when the program runs.

In GitLab by @leggettc18 on Oct 2, 2018, 13:42

Merges improve-output -> master

So this is kind of a two job commit, which I normally don't do, but I got a little carried away. Anyway this formats output so that things are lined up a bit more uniformly, which is much more aesthetically pleasing.
I also removed some unnecessary String cloning involving the get_by_fund_name method. It took a String, but it merely referenced it to get the Fund out of the HashMap, so I changed it to just take a &str instead. There are probably several places in the program I can do this in later on.

In GitLab by @leggettc18 on Sep 27, 2018, 18:19

/u/usernamedottxt on Reddit said

Your load function unwraps and panics on a parser error. Should return a result instead.

https://www.reddit.com/r/rust/comments/9jdjpc/my_new_project_in_rust_open_to_critiques_and/e6qno3f

In GitLab by @leggettc18 on Sep 27, 2018, 18:15

/u/usernamedottxt on Reddit said

Don't record money as a f64. Store pennies and convert to dollars on display. This is one of the mortal sins of programming in general.

https://www.reddit.com/r/rust/comments/9jdjpc/my_new_project_in_rust_open_to_critiques_and/e6qno3f

In GitLab by @leggettc18 on Oct 2, 2018, 15:09

Test coverage needs to be better. There are currently exactly 3 tests. If I want to make use of CI at some point I need to make sure every feature is verified for correctness by tests. Should be done after #10 since that is the last of the "basic" features I need to implement.

In GitLab by @leggettc18 on Oct 7, 2018, 18:30

In GitLab by @leggettc18 on Oct 7, 2018, 18:29

In GitLab by @leggettc18 on Sep 27, 2018, 18:11

From /u/usernamedottxt on Reddit

in your new function, you're already returning a boxed error. Why unwrap on opening the config and home directory? It's trivial to just return an IO error instead.

Need to investigate what the proper syntax for this would be. Apparently it shouldn't be complicated, I just don't know exactly what changes need to occur at this exact moment in time.

https://www.reddit.com/r/rust/comments/9jdjpc/my_new_project_in_rust_open_to_critiques_and/e6qno3f

In GitLab by @leggettc18 on Oct 2, 2018, 23:52

Merges remove-clones -> master

This MR removes as many clone and to_owned statements from the code as possible. By my count there are still 3 that I could not find a way around at this point in time. Perhaps I will re-visit those later, but for now, with the bulk of the clones gone, I need to work on test coverage and documentation.

Closes #9

In GitLab by @leggettc18 on Oct 10, 2018, 18:48

Merges impl-extend -> master

This MR implements the Extend trait on FundManager. I implemented it as part of the Rust API Checklist recommendations, and figured it could come in handy for any future libfund users. However, this comes with a "gotcha!". If you try to extend a FundManager and the iterator you supply contains any duplicate names, they will be ignored for now. This warning will be expressed in the Documentation both at the top and in the Trait implementation section.

In GitLab by @leggettc18 on Sep 27, 2018, 18:14

/u/usernamedottxt on Reddit said

This:

let goal = if goal.is_some() {
        Some(goal.unwrap().parse::<f64>()?)
    } else {
        None
    };

Can be written as:

let goal = goal.map_or(None, |x| x.parse::<f64>())?;

https://www.reddit.com/r/rust/comments/9jdjpc/my_new_project_in_rust_open_to_critiques_and/e6qno3f

In GitLab by @leggettc18 on Oct 7, 2018, 17:12

Merges builder-pattern -> master

I have managed to implement the builder pattern on the Fund struct. This allows for, IMO, a cleaner way of creating new funds that are missing an amount or goal value. Whereas before it would have looked like Fund::new(None, None) or Fund::new(Some(100), Some(500)) or some combination of the above. Adding a Fund to a FundManager now looks like this:

// assuming we already have a FundManager named funds
funds.add_fund("grocery", Fund::new().with_amount(100).with_goal(100).build();

Either or both of with_amount and with_goal can be omitted to go with the default value of 0. If either of them are used, you must use the build function to get an owned Fund rather than a mutable reference.

You can also do something like

let op_amount = Some(100);
let op_goal = Some(500);
let mut fund = Fund::new();
if let Some(amount) = op_amount {
    fund.with_amount(amount);
}
if let Some(goal) = op_goal {
    fund.with_goal(goal);
}
let fund = fund.build();

Closes #15

Which is useful for FundWarrior's case as the `Config` struct holds the non-required arguments from the command line as `Option<i32>`s.

In GitLab by @leggettc18 on Oct 7, 2018, 18:38

• Investigate which of the following traits are appropriate to implement for Fund and FundManager:
  Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash, Debug, Display, Default

Copy, Clone, PartialEq, Debug, Display, Default have already been implemented for Fund

• Implement the ones determined to be appropriate

In GitLab by @leggettc18 on Oct 2, 2018, 15:08

I think there should be a command for transferring money between funds. This would involve adding an extra command, parsing an extra argument, and writing a function to handle the transfer.

In GitLab by @leggettc18 on Oct 10, 2018, 20:55

Merges impl-traits -> master

Closes #25

Implements the following traits on Fund: Debug, Default, PartialEq, Eq, Hash, Copy, Clone, Ord, and PartialOrd

Implements the following traits on FundManager: Debug, Default, PartialEq, Eq, and Clone

In GitLab by @leggettc18 on Oct 3, 2018, 16:41

Merges cleanup-organize -> master

This MR includes some Code Cleanup and Organization. No functionality was changed, but required steps to access have changed slightly, so the version will be bumped to 0.6.0. Changes include

• Refactoring of code to remove warnings from the compiler
• Refactoring of code to remove warnings from clippy
• Automatic formatting of code with fmt
• (This is the big one) Separates the Fund and FundManager implementations into a separate library crate contained within the main project. This was done for a few reasons:
  • Better separation of concepts
  • Potentially allows for different front-ends to be made later on that re-use the same implementations

In GitLab by @leggettc18 on Oct 2, 2018, 14:38

Merges improve-config-errors -> master

Closes #1

In GitLab by @leggettc18 on Oct 7, 2018, 18:28

In GitLab by @leggettc18 on Oct 8, 2018, 15:28

Merges get-refactor -> master

The getter function previously had a very long, unnecessarily verbose name, and it returned a mutable reference, but there are plenty of situations where you don't need a mutable reference to Fund. This MR deprecates the old method and replaces it with two new ones.

1. fund which takes a name and returns an immutable reference to a Fund (wrapped in a Result).
2. fund_mut which is the same but returns a mutable reference.

This also better adheres to the recommended naming convention in the Rust API checklist

In GitLab by @leggettc18 on Oct 10, 2018, 16:50

Merges error-handling -> master

This MR includes code that adds more context to any errors that could be returned from various functions in libfund. It has an unfortunate consequence of complicating the binary crate's run function by changing lines of code that were once just converting strings into errors into instantiating a new std::io::Error and then converting it to a libfund::FundManagerError. There may be a better way to write these bits, and they may also not even be necessary, as clap will catch most of those errors before even reaching that point of the program.

The new error types introduced are

1. FundNotFoundError: for when fund and fund_mut cannot find a fund.
2. DuplicateFundError: for when add_fund attempts to add fund with a name that already exists, and when rename attempts to rename a fund to a name that already exists
3. FundManagerError: a wrapper around the previous two errors and std::io::Error. Any crate that utilizes libfund have to use functions that could return any combination of the three error variants it contains. The fund crate's run function returns this error type as various functions it calls could return any one of these variants.

I also added documentation to these new error types

Closes #21

In GitLab by @leggettc18 on Oct 7, 2018, 18:34

In GitLab by @leggettc18 on Sep 28, 2018, 20:43

I didn't add any way to make sure fund names are unique upon creation, so two funds with the same name can be added. The get_by_name function returns only one, whichever one it finds first, rendering one or the other inaccessible depending on how the Fund Vector is sorted. Implementing PartialEq on Fund would allow to check if a fund by the name already exists in the Fund Vector before adding it, and the Fund::new() function can return an error if it does.

In GitLab by @leggettc18 on Sep 28, 2018, 23:50

The fund list could be implemented as a HashMap with the name as the key and Fund as the value, rather than a vector. This could allow for faster lookup of funds by their name. May require significant amount of refactoring.

In GitLab by @leggettc18 on Oct 3, 2018, 19:47

I am considering replacing my current configuration setup with the one found here. The main issue is this may be a bit overkill but it doesn't seem to be overly complicated so it may not make much of a performance impact.

In GitLab by @leggettc18 on Oct 2, 2018, 15:04

I cloned several strings that could probably be sent as references instead of ending up owned later on, which would remove the need for cloning. This occurs at several places throughout the code, mostly in the run function, but in a few other places as well.

In GitLab by @leggettc18 on Oct 1, 2018, 18:09

Merges hash-map -> master

Changes implementation of FundManager to use a HashMap of funds rather than a Vector. This allows for faster name-based lookup of funds than previously, as the hasher function uses the name to find the exact entry rather than iterating over the entire list looking for a matching name. The name field of the Fund struct as also been removed, as it is no longer needed. The status-printing functions were changed so that they no longer use the name field that was on Fund

Closes #4 and #8

In GitLab by @leggettc18 on Oct 3, 2018, 21:39

Merges add-tests -> master

Rounds out the test coverage of libfund, so all its functions and features are tested and verified to work. Closes #11

In GitLab by @leggettc18 on Oct 5, 2018, 19:02

Merges add-docs -> master

Closes #12
This adds documentation comments to the libfund crate.

In GitLab by @leggettc18 on Oct 7, 2018, 18:19

The following is a list of features not yet implemented that I think should be implemented for this project to be worth publishing.

• Implement IntoIterator on FundManager
• Implement FromIterator on FundManager

These two may allow for simplification of the save and load functions.

• Implement a feature to set the goal or amount of a fund to a specific value after creation
• Adding more code examples to the documentation
• Fine tune error handling, creating a few custom Error types if necessary.
• Implementing Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash, Debug, Display, Default traits on Fund and FundManager where applicable.

These are other things worth considering, which are not strictly necessary right away, and may require some investigation into their usefulness what Rust's best practices are

• Having functions like print_all from the library crate return strings to the caller instead of printing to stdout
• Having separate functions for getting funds, one that gets a mutable reference, one that gets an immutable one, and perhaps some unsafe getter methods that don't check for errors first.
• Implementing a feature where each fund gets a certain percentage when a deposit happens that isn't delegated to a specific Fund (not sure if this should happen before 1.0.0 or come after in a 1.1.0 release).

This list may fluctuate if I think of anything else. I got some of these additions from things I've noticed while doing other fixes and enhancements, and some of them came from here.

In GitLab by @leggettc18 on Oct 8, 2018, 04:34

Merges impl-into-from-iter -> master

Since FundManager is basically just a wrapper around a HashMap, I decided it would be a good idea to allow it to be iterated over and and collected from an iterator as if it was a HashMap. Additionally, it is implemented in such a way that into_iter does not take ownership of the FundManager. This way, it can be iterated over and then saved to a file later on.

I still need to implement the mutable iterator, So #24 is feasible.

Closes #17 and #18

In GitLab by @leggettc18 on Oct 7, 2018, 18:26

In GitLab by @leggettc18 on Oct 7, 2018, 15:14

I recently found out more about the builder pattern, which I think could be applied to libfund to allow a more elegant solution than supplying Options as arguments. This may require a significant refactor however, so I need to experiment with it before I can make a decision.