Skip to content

Upgrade to dune#13

Open
baransu wants to merge 1 commit into
andrenth:masterfrom
baransu:master
Open

Upgrade to dune#13
baransu wants to merge 1 commit into
andrenth:masterfrom
baransu:master

Conversation

@baransu
Copy link
Copy Markdown

@baransu baransu commented Oct 23, 2019

Following PR makes the following changes:

  • updated to dune via dune upgrade
  • fixes many now default dune errors like unused variables, unused opens

Not sure if everything is correct. It was trial and error to get it working with new Reason + esy project.

Comment thread mysql/mysql.ml
let date ?(year = 0) ?(month = 0) ?(day = 0) = fun _ ->
Lit (Lit.Date { base_time with year; month; day })
let datetime ?(year = 0) ?(month = 0) ?(day = 0)
?(hour = 0) ?(minute = 0) ?(second = 0) = fun _ ->
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Shouldn't you actually use that value in { base_time with ... instead of removing it?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I removed it because it was unused. Good point tho.

@andrenth
Copy link
Copy Markdown
Owner

Hi

Thanks for the update, and sorry for the delay. Could you please do as @sagotch mentioned and use the hour parameter in the function instead of removing it? Also, there are some warnings about unused functions like to_string which are actually part of the interface; I believe the warning happens because there is no mli file.

You can try adding those, but it'll be a lot of work. For now I think we could instruct dune to ignore those warnings.

After those changes I can merge the PR.

@sagotch
Copy link
Copy Markdown

sagotch commented Nov 12, 2019

Also, there are some warnings about unused functions like to_string which are actually part of the interface; I believe the warning happens because there is no mli file.

  (* let to_string : type a b. (a, b) t -> string = function
    | Time (name, table) -> sprintf "%s.%s" (Table.name table) name
    | Timestamp (name, table) -> sprintf "%s.%s" (Table.name table) name
    | Date (name, table) -> sprintf "%s.%s" (Table.name table) name
    | Datetime (name, table) -> sprintf "%s.%s" (Table.name table) name
    | Enum (name, table, _) -> sprintf "%s.%s" (Table.name table) name
    | Null.Time (name, table) -> sprintf "%s.%s" (Table.name table) name
    | Null.Timestamp (name, table) -> sprintf "%s.%s" (Table.name table) name
    | Null.Date (name, table) -> sprintf "%s.%s" (Table.name table) name
    | Null.Datetime (name, table) -> sprintf "%s.%s" (Table.name table) name
    | Null.Enum (name, table, _) -> sprintf "%s.%s" (Table.name table) name
    | other -> to_string other *)

  let to_string fld =
    let t = table fld in
    sprintf "%s.%s" (Table.name t) (name fld)

to_string is (re)define and the first definition is actually unused.

@XVilka
Copy link
Copy Markdown
Contributor

XVilka commented Jan 9, 2020

Were there any updates on this PR? Would be nice to get this merged.

@baransu
Copy link
Copy Markdown
Author

baransu commented Jan 9, 2020

Unfortunately not. I was experimenting with this library and trying to make it work for me so I created this PR. I'm not sure I'll have time to finish this.

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.

4 participants