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

Generate Display impl for enums #643

Merged
merged 1 commit into from
Nov 16, 2018
Merged

Conversation

GuillaumeGomez
Copy link
Member

Part of #448.

I have still troubles to fully understand how to correctly do it for flags but it'll come as well.

cc @EPashkin @sdroege

src/codegen/enums.rs Outdated Show resolved Hide resolved
@@ -65,6 +65,7 @@ pub fn generate(env: &Env, root_path: &Path, mod_rs: &mut Vec<String>) {
file_saver::save_to_file(path, env.config.make_backup, |w| {
try!(general::start_comments(w, &env.config));
try!(general::uses(w, env, &imports));
try!(writeln!(w, "use std::fmt;"));
Copy link
Member

Choose a reason for hiding this comment

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

This better be done with imports.add

@EPashkin
Copy link
Member

@GuillaumeGomez Thanks, 👍

for member in &members {
try!(cfg_deprecated(w, env, member.deprecated_version, false, 3));
try!(version_condition(w, env, member.version, false, 3));
try!(writeln!(w, "\t\t\t{0}::{1} => \"{1}\",", enum_.name, member.name));
Copy link
Member

Choose a reason for hiding this comment

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

Two issues here

  1. It should be configurable if this generated, just like what I added for other types. So that it can be manually overridden.
  2. Use the "special functions" stuff from gir (just like we do for the various trait impls of other kinds of types). If there's e.g. a to_string() function, then this can be used here. I actually only thought (in the issue I created months ago) about creating the Display impl only if there's a special function for it.

Copy link
Member

Choose a reason for hiding this comment

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

Note: currently we don't generate enums and flags functions, so this need be done before, if we decide generate only for some enums.

Copy link
Member

Choose a reason for hiding this comment

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

That should happen because otherwise this is going to break the GStreamer bindings (where I have some custom Display impls) :)

@GuillaumeGomez
Copy link
Member Author

Updated.

Copy link
Member

@EPashkin EPashkin left a comment

Choose a reason for hiding this comment

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

Thanks. Now we can skip generation if needed.

src/codegen/enums.rs Outdated Show resolved Hide resolved
src/codegen/enums.rs Outdated Show resolved Hide resolved
src/codegen/object.rs Outdated Show resolved Hide resolved
src/codegen/object.rs Outdated Show resolved Hide resolved
@GuillaumeGomez
Copy link
Member Author

Updated.

@EPashkin
Copy link
Member

@GuillaumeGomez Thanks. Waiting CI and @sdroege confirmation

@sdroege
Copy link
Member

sdroege commented Nov 16, 2018

Seems fine to me

@EPashkin EPashkin merged commit 1b7c3da into gtk-rs:master Nov 16, 2018
@GuillaumeGomez GuillaumeGomez deleted the enum-display branch November 16, 2018 11:30
let generate_display_trait = toml_object
.lookup("generate_display_trait")
.and_then(|v| v.as_bool())
.unwrap_or(true);
Copy link
Member

Choose a reason for hiding this comment

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

@GuillaumeGomez Why does this default to true still?

Copy link
Member Author

Choose a reason for hiding this comment

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

By "default" we want to generate the Display trait. However, if we have an invalid value, would you prefer instead?

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer for it to be opt-in

Copy link
Member Author

Choose a reason for hiding this comment

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

@EPashkin opened #668. You might want to take a look at it. :)

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