Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Develop #10

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from
Open

Develop #10

wants to merge 11 commits into from

Conversation

nut-code-monkey
Copy link

Merge branch 'master' into develop

NSError as a result of validation/schema adding

samskiter and others added 11 commits March 6, 2014 14:26
# By Sam Duke
# Via Sam Duke (1) and samskiter (1)
* hotfix/docs:
  Update README.md
  Update README.md
* release/v0.2:
  bumped to version 0.2
  added personal email
  Correcting the Podspec, removing NSLogs and replacing them with assertions and/or boolean success returns on adding reference schemas.
  Updated Podspec
  Get rid of the goto
  Reorganizing the project for cleaner CocoaPods integration
  Added travis.yml for Travis build status.
  Added a CocoaPods pod spec -- email omitted
  Added extra assurance that the test suite is executing okay.
  Initializing some uninitialized variables.  Extracting the one/true false/zero checks for uniqueItems and refactoring them.  Both of these changes are in order to resolve a condition where the fudgeFactor calculation was falsified -- falseFound was already false (due to an uninitialized variable and an unreachable else-condition).  With Xcode 5.1 the unit tests are still passing now.  With an earlier version of this commit, Sam noted that the tests were failing, though I'm not sure if it's a variation due to differences in compiler versions (that would be scary).  Hopefully it was just a matter of an incomplete commit on my part.  Regardless, "it works fine for me" :-)
  Creating an NSNumberFormatter is one of the most expensive single-line methods in the Foundation framework (along with NSDateFormatter).  If I recall correctly, each time you create one, it allocates 100+KB of memory and takes several milliseconds to complete.  If you ever create one, make sure that you *only* create one, and then reuse it.  But preferably, just don't create one if it's not needed (as in this case).
  Use obj-c BOOL constants instead of C. Rename uppercase variables to camelCase.  Added a variable (schemaItem = schema[keyword]) to get more reuse out of the schema dictionary's objectForKey method.
  addRefSchema methods return boolean indicating success/failure.
  Fixed up the KiteValidationPair to have a good hash and description.  It also uses auto-synthesized ivars
  Renamed the test class/file and the Pair class/files.  Fixed some compiler warnings (Xcode 5.1) and annotated FIXME's for the static analyzer's discovery of garbage values in the uniqueItems fudgeFactor calculating conditions.
* release/v0.2.1:
  bump version to 0.2.1
  Update to latest test suite and fix unicode length failures.
  Update README.md
  Update README.md
  Update README.md
* release/v0.2.2:
  bump to v0.2.2
  switched on and fixed a bunch of warnings
  fixes for stricter warnings
Conflicts:
	Sources/KiteJSONValidator.h
	Sources/KiteJSONValidator.m
@dodikk
Copy link

dodikk commented Mar 16, 2015

@samskiter this patch is going to break backward compatibility. For the existing library users it may be like #define true false .

Still, I agree that NSError should be returned by the affected method.

// AS IS
-(BOOL)validateJSONData:(NSData*)jsonData withSchemaData:(NSData*)schemaData;

// from the patch
-(NSError*)validateJSONData:(NSData*)jsonData withSchemaData:(NSData*)schemaData;


// My proposal
// follows Cocoa conventions
-(BOOL)validateJSONData:(NSData*)jsonData 
         withSchemaData:(NSData*)schemaData
                  error:(NSError**)outError;

@samskiter
Copy link
Owner

@dodikk that's a great catch! thank you. My head is in Java mode at the minute and I forgot about the auto cast. also, this is actually the way i prefer to return errors if any - more consistent with apple's APIs and other libraries

@nut-code-monkey sorry for another delay on this PR.

@dodikk
Copy link

dodikk commented Mar 16, 2015

@samskiter , I think, these days I'll finally get a chance (in terms of time) to add your validator to my project. Eventually, I'll adopt these changes and remaster the API in the Cocoa way so that you'll get a patch from me.

P.S. @samskiter , I guess, this one is no longer relevant and can be closed. #9

@dodikk
Copy link

dodikk commented Mar 22, 2015

@samskiter spent some of my leisure time trying to make the API follow the cocoa style dodikk@7db8dae

I can't afford spending so much time copy-pasting the if (NULL != outError) thing and structuring "return" statements. So, I'm giving up for now. Sorry.

@dodikk
Copy link

dodikk commented Mar 22, 2015

It would be a lot easier if I could use NSAssert(NULL != outError) rather than if statements. It would makes the changes so much more simple.

Such policy makes sense since it will be a lot harder to ignore validation errors. @samskiter , what do you think?

@samskiter
Copy link
Owner

Hi @dodikk thanks very much for your efforts. this is on my TODO list to sort out properly. It does look a little smelly that we have to have a lot of checks agains NULL (this should probably be nil by the way). so I will have to take a closer look.

@dodikk
Copy link

dodikk commented Apr 1, 2015

It does look a little smelly that we have to have a lot of checks agains NULL (this should probably be nil by the way).

That's how out parameters are done in "plain old C". And yes, apple allows NULL errors in their own API.


this should probably be nil by the way

No, it should not. NSError* (one star) and NSError** (two stars) are two completely different types. http://nshipster.com/nil/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants