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

Making the type Name.domain_name abstract #52

Merged
merged 2 commits into from Apr 13, 2015

Conversation

@heidi-ann
Copy link
Contributor

commented Feb 17, 2015

This is my attempt to no longer expose that domain_name is implemented as a string list in the Name module, please review
Discussion about making domain_name abstract: #51

@@ -27,11 +27,20 @@ open Cstruct
type label

(** Domain name, as a list of strings. *)
type domain_name = string list
type domain_name

This comment has been minimized.

Copy link
@dsheets

dsheets Feb 17, 2015

Member

Would be nice to just call this t as we're breaking the interface anyway. Name.domain_name is a bit verbose.

This comment has been minimized.

Copy link
@heidi-ann

heidi-ann Feb 17, 2015

Author Contributor

i agree, will try to get this compiling, then do a big find replace

This comment has been minimized.

Copy link
@heidi-ann

heidi-ann Feb 18, 2015

Author Contributor

i'd prefer to do this change after this PR, otherwise it will pollute the diff files, making them difficult to read


(** Domain name map *)
module Map: Map.S with type key = domain_name

(** Create null domain name *)
val empty: domain_name

This comment has been minimized.

Copy link
@dsheets

dsheets Feb 17, 2015

Member

Prefer "empty" to "null" in doc. Could also call this object "root" as it's the zero-label list (parent of all names).

This comment has been minimized.

Copy link
@heidi-ann

heidi-ann Feb 17, 2015

Author Contributor

called in null 185ce2f

This comment has been minimized.

Copy link
@dsheets

dsheets Feb 17, 2015

Member

Oops, I meant in the documentation string "null" is a bit confusing. Does it mean using it is invalid? The word "empty" or "zero" or "root" is more descriptive, IMO.

This comment has been minimized.

Copy link
@heidi-ann

heidi-ann Feb 18, 2015

Author Contributor

fixed, now called empty in function name and doc

val empty: domain_name

(** Render a {! domain_name} to a {! string} list. *)
val domain_name_to_string_list : domain_name -> string list

This comment has been minimized.

Copy link
@dsheets

dsheets Feb 17, 2015

Member

Prefer "to_string_list" and comment like "[to_string_list name] is the list of labels in the name [name]"

This comment has been minimized.

Copy link
@heidi-ann

heidi-ann Feb 18, 2015

Author Contributor

now called to_string_list, i agree with preference for [ ] to {! } but I'll leave for another PR as this is consistent throughout atm

val domain_name_to_string_list : domain_name -> string list

(** Convert a standard domain {! string} list to a {! domain_name}. *)
val string_to_domain_name : string list -> domain_name

This comment has been minimized.

Copy link
@dsheets

dsheets Feb 17, 2015

Member

Prefer "of_string_list" and same doc style as above.


(** Convert a standard domain {! string} list to a {! domain_name}. *)
val string_to_domain_name : string list -> domain_name

(** Render a {! domain_name} to a simple string. *)
val domain_name_to_string : domain_name -> string

This comment has been minimized.

Copy link
@dsheets

dsheets Feb 17, 2015

Member

You should probably break these as well to make them match with the above style which is much preferred over the present style (imo).

This comment has been minimized.

Copy link
@heidi-ann

heidi-ann Feb 18, 2015

Author Contributor

i naively tried this, noticed how pervasive open Cstruct was gave up :)
commit 17f69f5
revert 1593fa7

@dsheets

This comment has been minimized.

Copy link
Member

commented Feb 17, 2015

I think this is the right approach. This whole interface could be improved if we're going to break compat by hiding the type alias to string list. I'd prefer to standardize the style of the interface and break as much as possible in a single release than dribble out changes and live with weird names forever.

@dsheets

This comment has been minimized.

Copy link
Member

commented Feb 17, 2015

Also, the open Cstruct at the beginning is annoying (makes t mean Cstruct.t when reading. If you update the interface, please add yourself to the file header.

@heidi-ann

This comment has been minimized.

Copy link
Contributor Author

commented Feb 17, 2015

So, I can't seem to get rid of the following type error, despite casting all the string lists to Name.domain_name

File "lib/zone_parser.mly", line 227, characters 70-72:
Error: This expression has type string list
       but an expression was expected of type Name.domain_name
Command exited with code 2.
@dsheets

This comment has been minimized.

@heidi-ann

This comment has been minimized.

Copy link
Contributor Author

commented Feb 18, 2015

@heidi-ann

This comment has been minimized.

Copy link
Contributor Author

commented Feb 18, 2015

im stalled on

File "lib/zone_parser.mly", line 364, characters 19-30:
Error: This expression has type string list
       but an expression was expected of type Name.domain_name

the types seem to propagate in strange ways in the yacc file

@dsheets

This comment has been minimized.

Copy link
Member

commented Feb 18, 2015

https://github.com/mirage/ocaml-dns/blob/master/lib/loader.mli#L38 looks like the RR adders take domain_name for owner but state.owner is string list.

@heidi-ann

This comment has been minimized.

Copy link
Contributor Author

commented Feb 18, 2015

thanks david, i'll switch owner to domain_name after the meeting

@heidi-ann heidi-ann changed the title Making domain_name abstract (NOT COMPLETE) Making domain_name abstract Feb 18, 2015

@heidi-ann heidi-ann changed the title Making domain_name abstract Making the type Name.domain_name abstract Feb 18, 2015

| label_except_at { Name.cons (Bytes.lowercase $1) state.origin }
| label DOT { Name.of_string_list ([Bytes.lowercase $1]) }
| label DOT domain_labels { Name.cons (Bytes.lowercase $1) (Name.append (Name.of_string_list $3) state.origin) }
| label DOT domain_labels DOT { Name.of_string_list ((Bytes.lowercase $1) :: $3) }

This comment has been minimized.

Copy link
@dsheets

dsheets Feb 18, 2015

Member

No need for Bytes.lowercase here any more.

This comment has been minimized.

Copy link
@heidi-ann

heidi-ann Feb 18, 2015

Author Contributor

done bef3460

@heidi-ann heidi-ann force-pushed the heidi-ann:qname-patch branch from bef3460 to 20b7563 Feb 18, 2015

@heidi-ann

This comment has been minimized.

Copy link
Contributor Author

commented Feb 18, 2015

@dsheets tada! now as one commit

| label_except_at { Name.cons $1 state.origin }
| label DOT { Name.of_string_list ([$1]) }
| label DOT domain_labels { Name.cons $1 (Name.append (Name.of_string_list $3) state.origin) }
| label DOT domain_labels DOT { Name.of_string_list $1 :: $3) }

This comment has been minimized.

Copy link
@dsheets

dsheets Feb 18, 2015

Member

Syntax error here.

Also, could you please make the commit message more descriptive? Something like "Make Name.domain_name abstract" maybe?

This comment has been minimized.

Copy link
@samoht

samoht Feb 18, 2015

Member

Syntax error here.

who needs a compiler when you have @dsheets to automatically compile your PR in his head? :p

This comment has been minimized.

Copy link
@heidi-ann

heidi-ann Feb 19, 2015

Author Contributor

syntax errors fixed and commit msg edited

@heidi-ann heidi-ann force-pushed the heidi-ann:qname-patch branch from 9f9981e to 9e65916 Feb 19, 2015

@heidi-ann heidi-ann force-pushed the heidi-ann:qname-patch branch from 9e65916 to d199e1b Feb 19, 2015

@infidel

This comment has been minimized.

Copy link
Contributor

commented Feb 19, 2015

When I checked out your PR and then typed "make" it didn't compile:

File "lib_test/unix/time_server.ml", line 28, characters 6-10:
Error: This expression has type string list
       but an expression was expected of type Dns.Name.domain_name
Command exited with code 2.

But yes, #54 needs to be fixed too.

@heidi-ann

This comment has been minimized.

Copy link
Contributor Author

commented Feb 19, 2015

@infidel thanks, I forgot to test on unix as well as xen backends. I'll fix soon

@heidi-ann

This comment has been minimized.

Copy link
Contributor Author

commented Mar 3, 2015

Sorry its taken me so long, I believe its working now

@dsheets dsheets merged commit 60dbc66 into mirage:master Apr 13, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
avsm pushed a commit to avsm/ocaml-dns that referenced this pull request Jun 23, 2017
Merge pull request mirage#52 from samoht/master
Revert async_ssl API changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.