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

Remove the first-class module API #293

Merged
merged 1 commit into from Sep 8, 2015

Conversation

talex5
Copy link
Contributor

@talex5 talex5 commented Sep 8, 2015

It's confusing to duplicate the API. See discussion at:

http://lists.xenproject.org/archives/html/mirageos-devel/2015-08/msg00049.html

Removing the duplication will also make other changes to the API easier.

val with_hrw_view :
(module S with type key = 'path) ->
(module VIEW with type t = 'view and type db = 'store and type key = 'path) ->
'store ->
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 S.t should be constraint to be of type 'store as well. Although it might be better to just remove that function and have a View.with_transaction function instead maybe? What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead, I've make path non-optional. Turns out that Key.empty was the only use of module S anyway, and it's quicker to pass an empty path than a store anyway.

Moving to View might make sense, but perhaps a job for another patch?

@samoht
Copy link
Member

samoht commented Sep 8, 2015

Thanks! Just a minor remark and I'm happy to merge.

@samoht
Copy link
Member

samoht commented Sep 8, 2015

Hum Travis is extremely slow ...

samoht added a commit that referenced this pull request Sep 8, 2015
@samoht samoht merged commit 33a03f6 into mirage:master Sep 8, 2015
@samoht
Copy link
Member

samoht commented Sep 8, 2015

Thanks!

@samoht
Copy link
Member

samoht commented Sep 8, 2015

Some wiki pages might need to be updated as well...

@dbuenzli
Copy link

dbuenzli commented Sep 9, 2015

Some wiki pages might need to be updated as well...

Which makes a case for always having all the docs in the mlis. That way you can easily review and force contributors to update documentation before accepting their patches (also with the advent of codoc, I will have little motivation to checkout individual, crappy, outdated project wikis). Always put documentation on par with code.

@samoht
Copy link
Member

samoht commented Sep 9, 2015

Done in https://github.com/mirage/irmin/wiki/_compare/e5f23567e3875c253eb0fe1a4062e38b531358d4

and yes, that API page should be merged in the mli at one point, it just there to help people contribute (which serves well its role)

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.

None yet

3 participants