Skip to content

Modernize - #149

Merged
zserge merged 12 commits into
masterfrom
modernize
Apr 20, 2019
Merged

Modernize#149
zserge merged 12 commits into
masterfrom
modernize

Conversation

@zserge

@zserge zserge commented Nov 29, 2018

Copy link
Copy Markdown
Owner

After years of inactivity, I decided to keep jsmn up-to-date with the modern expectations of how a tiny library should look like.

So far API has not been changed, but jsmn is now a single-header, header-only library, with fixed formatting rules and a linter (that already complains a lot).

@zserge

zserge commented Nov 29, 2018

Copy link
Copy Markdown
Owner Author

@pt300 Hello, and pleased to meet you. You seem to be the most active commenter in the issue tracker and de-facto maintainer, so I would like to ask you to review it. Sorry for being silent for too long.

@pt300

pt300 commented Dec 1, 2018

Copy link
Copy Markdown
Collaborator

I'd say that's an interesting approach.
I'm myself not a fan of putting code inside header files as that's not the place for it, but in this case it actually seems to work nicely with JSMN considering it's intended use case and size. Doing it like that already throws out the window problem of binary incompatibility revolving around the JSMN_PARENT_LINKS as users are forced to compile JSMN with their project. Moreover it better fits the usual workflow when programming devices like 8bit microcontrollers where libraries are compiled along with main code instead of being used as separate precompiled binaries.

My thought on that for now is that it's not that bad of an idea but I have mixed feelings about it. Would like to hear other opinions.

@rspencer01

Copy link
Copy Markdown

In my (limited) experience, header-only libraries are great and semistandard now for small libraries. The format means that they are very easily integrated into existing projects (don't need to worry about compiling and linking them in) and any tiny tweaks that the project may require of the library are easily implemented.

If you want an opinion, here is one: do it. 😄

@pt300

pt300 commented Mar 6, 2019

Copy link
Copy Markdown
Collaborator

Considering this change will be quite big I'd suggest going over currently open pull requests and consider incorporating changes such as #142 or #143

@circulosmeos

Copy link
Copy Markdown

Hi! I've just used the modernized version (flawlessly) in my project
👍

@zserge
zserge merged commit fdcef3e into master Apr 20, 2019
@Nelson-numerical-software

Copy link
Copy Markdown

Hi, do you plan to release an new version ? Thanks

@pt300

pt300 commented Apr 22, 2019

Copy link
Copy Markdown
Collaborator

What do you mean?

@Nelson-numerical-software

Copy link
Copy Markdown

Current version with tag in github: v1.0.0
This good feature requires a new tag :)

@pt300

pt300 commented Apr 24, 2019

Copy link
Copy Markdown
Collaborator

touché

@pt300

pt300 commented Apr 24, 2019

Copy link
Copy Markdown
Collaborator

I'll wait for @zserge to decide how exactly this should be handled.

@zserge

zserge commented Apr 24, 2019

Copy link
Copy Markdown
Owner Author

@pt300 Since we don't have any "version" variables/macros in the actual header file or elsewhere in the sources - the only versioning we have is a git tag. So, let's put a tag then.
To me this PR looks like an API-compatible change (all function signatures and data structs remained very much the same). So my suggestion is to make a v1.1.0 tag on the current HEAD.

@pt300

pt300 commented Apr 26, 2019

Copy link
Copy Markdown
Collaborator

But the way how library is used changed. I think that should be taken into account.

@pt300 pt300 mentioned this pull request Oct 10, 2019
@ghost ghost mentioned this pull request Nov 6, 2019
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.

5 participants