z_string - make separate app in deps #444

Closed
mworrell opened this Issue Oct 9, 2012 · 19 comments

Comments

Projects
None yet
5 participants
Owner

mworrell commented Oct 9, 2012

There is a fix for z_string here: https://github.com/Motiejus/z_string

I have merged his pull request to our version.

It might be good to move z_string out of our core altogether and make it a separate dependency.
We might also want to add the PropEr tests from the above repo.

Advantages:

  • More people will use it (it is quite useful)
  • We might get more fixed / additions

Opinions?

Owner

mmzeeman commented Oct 9, 2012

+1

Maybe it is nice to bundle these with other handy utility libs:

z_debug with the debug macro's
z_utils (basically all of them except the functions needing stuff from the zotonic context)
z_convert

I always miss them when I do non zotonic work.

Maas

There is a fixed version of z_string here: https://github.com/Motiejus/z_string

I have merged his pull request to our version.

It might be good to move z_string out of our core altogether and use this repo (or a fork of it).

Advantages:

More people will use it (it is quite useful)
We might get more fixed / additions
Opinions?


Reply to this email directly or view it on GitHub.

Owner

mworrell commented Oct 9, 2012

That is a good idea.

deps/z_utils ?

Owner

mworrell commented Oct 9, 2012

@arjan @kaos Your opinion please.

Owner

kaos commented Oct 9, 2012

I agree with moving erlang utility stuff into it's own lib. That could be used in other projects (and get a bigger community on itself).

But perhaps lump it all together, into one lib, into which we could put string, conversion, debug, and various other utility stuff.

I'm ok with z_utils. Another idea is z_stdlib (think: zotonic addendum to erlang's stdlib).

Owner

arjan commented Oct 9, 2012

+1 for z_stlib

However we'll need to see if we can collaborate with @Motiejus on this?

Contributor

Motiejus commented Oct 9, 2012

Sounds nice.

I had a look at these libraries, they are indeed useful. I would be glad to have a library which has z_utils + z_convert + z_string utility functions bundled .

However, debug macros are very application specific, so I would vote against including them to the bundle. And they are trivial to write in the project anyway.

Owner

mworrell commented Oct 15, 2012

I have split off https://github.com/zotonic/z_stdlib

Still some things to clean up and add.

(Think z_utils and our markdown stuff)

mworrell was assigned Oct 15, 2012

Owner

mworrell commented Oct 15, 2012

Now it contains:

  • z_string
  • z_convert
  • z_dateformat (from erlydtl_dateformat)
  • z_tempfile
  • z_url
  • z_html
Contributor

Motiejus commented Oct 16, 2012

Dependency on mochiweb? Why? Can it be avoided? It is really not good
for a generic library like this.

Also, what about PropEr tests? Are you going to include them? Since
PropEr is necessary only for running tests, we can embed PropEr only
while running tests:
https://github.com/Motiejus/erlualib/blob/master/Makefile
https://github.com/Motiejus/erlualib/blob/master/rebar_test.config

In other words, have a separate rebar.config for eunit.

Owner

arjan commented Oct 16, 2012

mochiweb is there for its HTML parser. I agree this is a bit hash... but we really want HTML sanitizing in this library...

Owner

arjan commented Oct 16, 2012

@Motiejus I'll do the split like you suggest, a rebar_test.config for proper/eunit tests.

Contributor

Motiejus commented Oct 16, 2012

Makes sense.

Maybe it is possible to extract HTML parser from mochiweb (and
optionally help modularize mochiweb that way, too)?

Consider the following:
my project depends on mochiweb 2.3.4-pre1 (assume it exists)
I want z_string:validate_utf8, and include z_stdlib to my project dependencies.

z_stdlib depends on other version of mochiweb, and we have a conflict,
which could be avoided.

z_stdlib could depend on:
{mochi_html, "", {git,
"git://github.com/mochiweb/mochi_html.git", "HEAD"}}

HEAD means "I'm fine with whatever is there".

In principle, it can be done the same with this project. However,
including full mochiweb really sounds like an overkill to such a slim
and otherwise nice library.

Alternatively, we can give user a choice: if user wants to use html
parser, she includes mochiweb by herself. If not, then html parsers in
z_stdlib would be no-ops.

Owner

arjan commented Oct 16, 2012

On 10/16/2012 01:45 PM, Motiejus Jakštys wrote:

Makes sense.

Maybe it is possible to extract HTML parser from mochiweb (and
optionally help modularize mochiweb that way, too)?

Consider the following:
my project depends on mochiweb 2.3.4-pre1 (assume it exists)
I want z_string:validate_utf8, and include z_stdlib to my project
dependencies.

z_stdlib depends on other version of mochiweb, and we have a conflict,
which could be avoided.

z_stdlib could depend on:
{mochi_html, "", {git,
"git://github.com/mochiweb/mochi_html.git", "HEAD"}}

HEAD means "I'm fine with whatever is there".

In principle, it can be done the same with this project. However,
including full mochiweb really sounds like an overkill to such a slim
and otherwise nice library.

Alternatively, we can give user a choice: if user wants to use html
parser, she includes mochiweb by herself. If not, then html parsers in
z_stdlib would be no-ops.

I agree. Let's do it this way: remove the mochiweb dependency from
rebar.config. Zotonic is now actually broken because of this :)

Although, I will add it to rebar.test.config because I need it for the
unit tests.


Reply to this email directly or view it on GitHub
#444 (comment).

Owner

mworrell commented Oct 16, 2012

We need the Zotonic version anyway, as there are some patches that aren't merged upstream yet.

I feel a bit for the "make a mochiweb_lib project" idea.
Though I guess we can't use the trademarked name Mochi for this :)

Owner

arjan commented Oct 16, 2012

But who is going to maintain it...

IIRC there are already a few mochiweb-splitoff projects out there.

Contributor

Motiejus commented Oct 16, 2012

After some more thought I think we can just leave it like this, and, if HTML parser is needed, the parent project (which imports z_stdlib) can include mochiweb as a dependency, whatever version xe likes.

For maintenance of mochilib. @arjan, you are right. It is serious. But I do not see a good need for it any more if the parent project is including mochiweb (or not). Since z_stdlib remains lightweight.

As for dialyzer and xref, there are ways to make them not complain for this particular instance.

Owner

arjan commented Oct 16, 2012

I implemented the changes I proposed
https://github.com/zotonic/z_stdlib

It's continuosly tested: https://travis-ci.org/#!/zotonic/z_stdlib

@Motiejus I ported your tests over to this repository, and it also uses PropEr now. Let us know if you want to become a committer on z_stdlib! :-)

Build Status

Contributor

Motiejus commented Oct 16, 2012

@arjan it looks much more self-contained now, thanks!

I am now working on a PropEr utf8 generator. Once it is done, it will be merged upstream:
manopapad/proper#48
It is much more difficult to make a correct utf8(Len) generator than I thought initially, but doable. Once it is done and merged to PropEr, we will be able to remove utf8 generator from z_string_sanitize_utf8_test.erl and depend on the one which will be in PropEr.

I also plan to add implicit xref support to z_stdlib at some point in time.

So yes, push access would save you from a couple of pull requests. :)

Owner

mworrell commented Nov 8, 2012

I close this ticket now, as the split of z_string to z_stdlib has been done.

mworrell closed this Nov 8, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment