bradyt / cone

A ledger.
https://cone.tangential.info
20 stars 5 forks source link

Set date format by reading dates from the ledger file #55

Closed justinjk007 closed 4 years ago

justinjk007 commented 4 years ago

I am only looking for date format like this yyyy/MM/dd if that doesn't exist I fall back to yyyy-MM-dd. Hopefully these formats are enough for now.

bradyt commented 4 years ago

Thank you @justinjk007,

This looks good. I haven't had a workflow to hack on this app in a few months, so I need to look at some things when I get enough time, but I hope to give this a complete review soon.

Note, I haven't added widget tests, nor any sort of CI testing, or much automation to existing tests, so I will have to tread carefully.

I'd also like to improve how easy it is to have two easily distinguishable apps installed, one for using, and one for testing.

The commit as is, might be the best thing to commit to master, so that as it stands, it will be easy to read and catch issues if any arise. But in the long run, I may, after this commit stands the test of time, end up refactoring, so instead of being in FileModel, you can test the existing chunks, and potentially update the FormatModel. I'm sure I'm missing something there, but that would be exactly why I want to take a longer look at this.

EDIT: chunks is probably not even a good suggestion, as really its current usage might be improved. I didn't mean to leave so much ad-hoc parsing in the home.dart file. Perhaps better if the ad-hoc parsing was completed in cone_lib/. Then the existence of a yyyy/mm/dd transaction could be a simple bool provided by cone_lib/. But again, I don't consider that blocking for your pull request. Just a preview of ideas for future changes.

bradyt commented 4 years ago

@justinjk007, why do you have these lines in your commit?

+          caseSensitive: false,
+          multiLine: false,
justinjk007 commented 4 years ago

It was in the example when I tried to lookup syntax for regex matching, we don't need them I think. I will remove those add the regex suggestion .

justinjk007 commented 4 years ago

Note, I haven't added widget tests, nor any sort of CI testing, or much automation to existing tests, so I will have to tread carefully.

I have setup CodeMagic CI for one projects of my before that was pretty simple. I am guilty of never writing widget tests too. I do want to learn to write them though, I will chip in some as we go along.

bradyt commented 4 years ago

Hmm, maybe we need multiLine: true,.

From https://api.dart.dev/stable/2.7.0/dart-core/RegExp/RegExp.html:

If multiLine is enabled, then ^ and $ will match the beginning and end of a line, in addition to matching beginning and end of input, respectively.

justinjk007 commented 4 years ago

But we just need to match one line anyways? By adding ^ aren't we expecting the beginning of our input to be a date anyways? It might be useful if someone need to get all the matches, for our usecase we only need to match once.

On Sun, Jan 12, 2020, 10:23 PM Brady Trainor notifications@github.com wrote:

Hmm, maybe we need multiLine: true,.

From https://api.dart.dev/stable/2.7.0/dart-core/RegExp/RegExp.html:

If multiLine is enabled, then ^ and $ will match the beginning and end of a line, in addition to matching beginning and end of input, respectively.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bradyt/cone/pull/55?email_source=notifications&email_token=ADCRQO2OM67IIUVSI7NKYXLQ5PNDFA5CNFSM4KFZ6HAKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIXNN5Y#issuecomment-573495031, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADCRQOYPJK2HHOMIWDW4CRDQ5PNDFANCNFSM4KFZ6HAA .

bradyt commented 4 years ago

You can try the following at https://dartpad.dev/.

void main() {

  print(RegExp(r'^blah').hasMatch('''
first line
blah'''));
  // false

  print(RegExp(r'^blah', multiLine: true).hasMatch('''
first line
blah'''));
  // true

}
bradyt commented 4 years ago

@justinjk007 I think the r'^[0-9]+/' won't work without mulitLine: true, with the following file example.

;; a comment
2020/01/01 an example
  a    5.00 EUR
  b

I think the only way the current code would work, is if the first line of the entire file had a transaction with a date with forward slashes.

I think you'll see that multiLine: true fixes this.

bradyt commented 4 years ago

This all looks good. Can we rebase as one commit on master?

justinjk007 commented 4 years ago

Squashed all the commits into one.

bradyt commented 4 years ago

Great, I've rebased to master and pushed. Apparently Github is broken, so it does not know this "pull request" has been "merged". Closing manually.

Thank you for the fix!