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

RFC: Not so twisted snakes. #586

Merged
merged 1 commit into from Apr 19, 2018

Conversation

Projects
None yet
4 participants
@tmiasko
Contributor

tmiasko commented Apr 18, 2018

Improve to_snake implementation. For example:

type name old module name new module name
DBusMessage d_bus_message dbus_message
FileIOStream file_i_o_stream file_iostream
IMContext i_m_context imcontext
IOStream i_o_stream iostream
URIHandler u_r_i_handler urihandler
UnixFDList unix_f_d_list unix_fdlist

Also remove dependency on case crate now that it is no longer needed.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Apr 19, 2018

Member

IMHO very good idea, even if it produces big changes.
Need rebase to solve merge conflict.

cc @GuillaumeGomez, @sdroege and @antoyo

Member

EPashkin commented Apr 19, 2018

IMHO very good idea, even if it produces big changes.
Need rebase to solve merge conflict.

cc @GuillaumeGomez, @sdroege and @antoyo

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Apr 19, 2018

Member

Seems good to me but if we change this let's please follow what is done in C a bit closer. It's io_stream, im_context, uri_handler, unix_fd_list. DBus is the odd one here with GDbus but g_dbus instead of g_d_bus

Member

sdroege commented Apr 19, 2018

Seems good to me but if we change this let's please follow what is done in C a bit closer. It's io_stream, im_context, uri_handler, unix_fd_list. DBus is the odd one here with GDbus but g_dbus instead of g_d_bus

@tmiasko

This comment has been minimized.

Show comment
Hide comment
@tmiasko

tmiasko Apr 19, 2018

Contributor

In this case, some further ideas:

  1. Insert underscore in-between characters when going from lowercase to uppercase.
  2. Version 1, but also insert underscore in-between second last and last
    uppercase character when going from sequence of three or more uppercase
    characters to lowercase.

Version 1 is what was proposed first, version 2 is some further refinement of
what I think @sdroege had in mind. The part "three or more uppercase characters"
is intended to avoid the case of single letter delimited with underscores.
If this don't feel to special casey to you, I would propose version 2.

Contributor

tmiasko commented Apr 19, 2018

In this case, some further ideas:

  1. Insert underscore in-between characters when going from lowercase to uppercase.
  2. Version 1, but also insert underscore in-between second last and last
    uppercase character when going from sequence of three or more uppercase
    characters to lowercase.

Version 1 is what was proposed first, version 2 is some further refinement of
what I think @sdroege had in mind. The part "three or more uppercase characters"
is intended to avoid the case of single letter delimited with underscores.
If this don't feel to special casey to you, I would propose version 2.

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Apr 19, 2018

Member

Do we have other examples of two uppercase characters in a row, other than DBus?

Member

sdroege commented Apr 19, 2018

Do we have other examples of two uppercase characters in a row, other than DBus?

@tmiasko

This comment has been minimized.

Show comment
Hide comment
@tmiasko

tmiasko Apr 19, 2018

Contributor

In trailing position: SoupCookieJarDB

Contributor

tmiasko commented Apr 19, 2018

In trailing position: SoupCookieJarDB

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Apr 19, 2018

Member

That's soup_cookie_jar_db, all good then. 2) seems like a good approach

Member

sdroege commented Apr 19, 2018

That's soup_cookie_jar_db, all good then. 2) seems like a good approach

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Apr 19, 2018

Member

I like the idea and I agree with @sdroege.

Member

GuillaumeGomez commented Apr 19, 2018

I like the idea and I agree with @sdroege.

Not so twisted snakes.
Improve to_snake implementation. For example:

Type name    | Old module name | New module name
-------------|-----------------|----------------
DBusMessage  | d_bus_message   | dbus_message
FileIOStream | file_i_o_stream | file_io_stream
IMContext    | i_m_context     | im_context
IOStream     | i_o_stream      | io_stream
URIHandler   | u_r_i_handler   | uri_handler
UnixFDList   | unix_f_d_list   | unix_fd_list

Also remove dependency on case crate now that it is no longer needed.
@tmiasko

This comment has been minimized.

Show comment
Hide comment
@tmiasko

tmiasko Apr 19, 2018

Contributor

I pushed updated implementation. It includes some additional test cases that
you might find interesting, especially those from GStreamer plugins. I think it
does agreeable job there, even in cases where it doesn't match what is used in
C code exactly.

For the record, new comparison table:

Type name Old module name New module name
DBusMessage d_bus_message dbus_message
FileIOStream file_i_o_stream file_io_stream
IMContext i_m_context im_context
IOStream i_o_stream io_stream
URIHandler u_r_i_handler uri_handler
UnixFDList unix_f_d_list unix_fd_list
Contributor

tmiasko commented Apr 19, 2018

I pushed updated implementation. It includes some additional test cases that
you might find interesting, especially those from GStreamer plugins. I think it
does agreeable job there, even in cases where it doesn't match what is used in
C code exactly.

For the record, new comparison table:

Type name Old module name New module name
DBusMessage d_bus_message dbus_message
FileIOStream file_i_o_stream file_io_stream
IMContext i_m_context im_context
IOStream i_o_stream io_stream
URIHandler u_r_i_handler uri_handler
UnixFDList unix_f_d_list unix_fd_list
@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez
Member

GuillaumeGomez commented Apr 19, 2018

👍

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Apr 19, 2018

Member

Looks good to me 👍

Member

sdroege commented Apr 19, 2018

Looks good to me 👍

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Apr 19, 2018

Member

Let merge then.
@tmiasko Thanks.
Later I will add here list of affected crates.

Member

EPashkin commented Apr 19, 2018

Let merge then.
@tmiasko Thanks.
Later I will add here list of affected crates.

@EPashkin EPashkin merged commit dda4a18 into gtk-rs:master Apr 19, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Apr 19, 2018

Member

I would prefer if we could get my glib/gio PRs merged before moving around all the files. The merge conflicts will be annoying to solve :)

Member

sdroege commented Apr 19, 2018

I would prefer if we could get my glib/gio PRs merged before moving around all the files. The merge conflicts will be annoying to solve :)

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Apr 19, 2018

Member

Fine by me. :)

Member

GuillaumeGomez commented Apr 19, 2018

Fine by me. :)

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Apr 19, 2018

Member

Affected crates: gtk, gio, gdk

Member

EPashkin commented Apr 19, 2018

Affected crates: gtk, gio, gdk

vhdirk pushed a commit to vhdirk/gir that referenced this pull request Jul 6, 2018

Merge pull request #586 from tmiasko/case
RFC: Not so twisted snakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment