Add option to disable automatic date parsing#1130
Add option to disable automatic date parsing#1130Belphemur merged 2 commits intoBelphemur:developfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1130 +/- ##
========================================
Coverage 99.60% 99.60%
========================================
Files 12 12
Lines 1527 1531 +4
Branches 322 322
========================================
+ Hits 1521 1525 +4
Misses 6 6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
This pull request adds a useful option to disable automatic parsing of date strings. The implementation is straightforward. However, the pull request is missing unit tests for the new functionality, which is a significant concern, especially since the description states that testing was not performed. I've added a comment highlighting the need for tests. I also have a couple of smaller suggestions: one to improve the example in the README.md to better demonstrate the new feature, and another for a minor code improvement in JsonAdapter.ts for regex matching.
| constructor(adapter: IAdapter<string>, humanReadable: boolean = false, parseDates: boolean = true) { | ||
| this.adapter = adapter; | ||
| this.humanReadable = humanReadable; | ||
| this.parseDates = parseDates; | ||
| } |
There was a problem hiding this comment.
README.md
Outdated
| // The fourth argument is the separator. By default it's slash (/) | ||
| // The fifth argument enables sync writes (default false) | ||
| // The sixth argument controls automatic Date parsing for ISO strings (default true) | ||
| var db = new JsonDB(new Config("myDataBase", true, false, '/', false, true)); |
There was a problem hiding this comment.
To better illustrate the new feature, it would be more helpful to show an example where date parsing is disabled (false). The current example uses true, which is the default and doesn't showcase the new capability.
| var db = new JsonDB(new Config("myDataBase", true, false, '/', false, true)); | |
| var db = new JsonDB(new Config("myDataBase", true, false, '/', false, false)); |
src/adapter/data/JsonAdapter.ts
Outdated
|
|
||
| private reviver(key: string, value: any): any { | ||
| if (typeof value == "string" && this.dateRegex.exec(value) != null) { | ||
| if (this.parseDates && typeof value == "string" && this.dateRegex.exec(value) != null) { |
There was a problem hiding this comment.
For checking if a regex matches a string, it's more idiomatic and slightly more performant to use RegExp.prototype.test() instead of RegExp.prototype.exec(). test() returns a boolean and doesn't create an array of matches.
| if (this.parseDates && typeof value == "string" && this.dateRegex.exec(value) != null) { | |
| if (this.parseDates && typeof value == "string" && this.dateRegex.test(value)) { |
|
Follow-up (clean version): addressed in commit 1573d38. Changes made:
Validation run:
|
|
Correction (previous comment formatting got mangled by my CLI quoting): Addressed in follow-up commit 1573d38:
Validation run (pass): npm test -- --runInBand test/adapter/adapters.test.ts |
|
Hello @SergioChan, Thanks for your contribution, I'm adding to the lib :) @all-contributors add @SergioChan for code |
|
I've put up a pull request to add @SergioChan! 🎉 |
Summary
Testing
Fixes issue #486