Skip to content
This repository has been archived by the owner on Jun 2, 2024. It is now read-only.

Json format: refactor error handling + add JSON_UNESCAPED_UNICODE #25

Merged
merged 4 commits into from
Sep 3, 2018

Conversation

assemblie
Copy link

hi! :) i really like your library so i of your ok with it i could try to implement some new features like new formats, filesystem abstraction, validation abstraction so it could use json schema (http://json-schema.org/) etc.
& refactor some of the functionality

i just wanted to add JSON_UNESCAPED_UNICODE to pretty print but decided to give Format a little refactor

hit me if your ok with it and ill implement Reading/Saving Exceptions + add tests

@coveralls
Copy link

Pull Request Test Coverage Report for Build 113

  • 17 of 33 (51.52%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-2.6%) to 90.223%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/Database.php 8 10 80.0%
src/Format/FormatException.php 0 4 0.0%
src/Format/Json.php 9 19 47.37%
Totals Coverage Status
Change from base Build 112: -2.6%
Covered Lines: 323
Relevant Lines: 358

💛 - Coveralls

@coveralls
Copy link

coveralls commented Aug 28, 2018

Pull Request Test Coverage Report for Build 117

  • 35 of 36 (97.22%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 92.955%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/Format/FormatException.php 3 4 75.0%
Totals Coverage Status
Change from base Build 112: 0.1%
Covered Lines: 475
Relevant Lines: 511

💛 - Coveralls

@timothymarois
Copy link
Member

@assemblie Nice job. This looks good to me. And yes, we welcome all improvements/features :)

src/Database.php Outdated

if (!$file) {
/**
* FIXME: shouldn't we raise an exception in this case? + implement add/create method?
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if its ok from users viewpoint that get (read) method returns empty document object if file doesn't exists? seems weird
maybe we should throw en exception in get (read) and implement add/create, getOrCreate or something like that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the initial idea was to provide production sites with safety in case of failure so that you could create new docs without checking if old existed. That's why I built the method has() which checks if the document exists.

An issue like this came up before where things created when I didn't want them too, so there is a config option for read_only so that it only reads the documents that exist. (of course thats not good if you do want to make updates, so its a narrow use-case. But yeah it's not perfect, and could use a bit more transparency for the developer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But as to your point, the get() was designed to get a document (from the file system) or an empty document if doenst exist. But only creates a new document in the filesystem if you call the save() method. This way you can get all the docs you want and then populate data, so if it exists or not intially, it can be created or updated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@assemblie Do you have a way to chat/talk. Like email, or slack etc? This way we can discuss without having to comment each time? really need a github messaging system...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timothymarois ok i get it imo this approach is ok but we need to state this in documentation explicitly and yes if you have slack workspace invite me here [email protected]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@assemblie Agreed. and invite sent.

@timothymarois timothymarois merged commit c7b426a into tmarois:1.0 Sep 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants