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

✨ zx: add --output option #499

Merged
merged 4 commits into from
Nov 30, 2023
Merged

Conversation

FineFindus
Copy link
Contributor

@FineFindus FineFindus commented Oct 28, 2023

Adds a new -output option with three different behaviors:

  • if a path is given, all output is written to the file
  • if - is given, the generated code is redirected to stdout
  • If the argument is omitted, the interfaces are split into individual files.

Copy link
Contributor

@zeenix zeenix left a comment

Choose a reason for hiding this comment

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

LGTM aside from 1 nitpick.

zbus_xmlgen/src/main.rs Outdated Show resolved Hide resolved
@zeenix
Copy link
Contributor

zeenix commented Oct 31, 2023

@FineFindus oh and the CI is failing.

@zeenix
Copy link
Contributor

zeenix commented Oct 31, 2023

@FineFindus oh and the CI is failing.

Oh, looks like that's unrelated to your commits. Please rebase on current main and it should be fine. Windows one might need a rerun though. For some reason, the pkg-config download from sourceforge fails often. 🤷

Copy link
Contributor

@zeenix zeenix left a comment

Choose a reason for hiding this comment

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

Are the double shortlogs in the last commit on purpose?

 ✨ zx: add split interfaces option

♻️ zx: clone service and path

@FineFindus
Copy link
Contributor Author

No real reason, just git deciding to add it automatically when squashing and me not noticing :). Fixed it.

@zeenix
Copy link
Contributor

zeenix commented Nov 6, 2023

No real reason, just git deciding to add it automatically when squashing and me not noticing :). Fixed it.

Thanks! but could you add some description there? Feel free to c&p the PR description.

Copy link
Contributor

@zeenix zeenix left a comment

Choose a reason for hiding this comment

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

I also now notice an inconsistency here. Normally the generated code is just thrown on the console, except (now) when --split-interfaces is specified. Perhaps, it'd be better and we make --split-interfaces implied by default? We can add a different option --output (short form: -o) to output to a specific file. If user specifies -o -, we output everything to the stdout. This will also be consistent with how other tools (e.g gdbus) work I think. WDYT?

zbus_xmlgen/src/main.rs Outdated Show resolved Hide resolved
@FineFindus
Copy link
Contributor Author

Frankly, I'm not a big fan of creating a bunch of files without explicit user permission. This is especially the case when you accidentally run the command (e.g. to try something out) and now have to clean up all the newly created files.
So using --split-interfaces without -o would just print the interfaces to stdout as it does now, and only create files if -o is specified?

@zeenix
Copy link
Contributor

zeenix commented Nov 26, 2023

Frankly, I'm not a big fan of creating a bunch of files without explicit user permission.

  • It's not a bunch of random files but only as many files as there are interfaces included in the interface xml that user explicitly pointed us to for the sole reason of creating code they can use.
  • Sine we're being frank, your or my personal preferences are not as relevant as a good UX. :) We should focus on what the user would need and their ease of use. Typically (I'd say 99% cases at least), user would need to put the generated code into a file. As long as they've an easy way to override the default behavior, I think we're good.
  • Moreover, where possible, we should keep the UX compatible with existing tools that people are used to and have no complaints against. gdbus-codegen doesn't seem to even have an option to split the output though but it does output to a file.

@zeenix
Copy link
Contributor

zeenix commented Nov 26, 2023

This is especially the case when you accidentally run the command (e.g. to try something out) and now have to clean up all the newly created files.

That's not our problem to solve really. They could run the command accidently with --split-interfaces as well.

So using --split-interfaces without -o would just print the interfaces to stdout as it does now,

What's --split-interfaces doing then? It's been just ignored and that's very unexpected. Even if we keep the current behavior, this should result in an error then. I really think we should change the default behavior as that would mean everything is consistent.

@FineFindus FineFindus changed the title ✨ zx: add --split-interfaces option ✨ zx: add --output option Nov 28, 2023
@FineFindus FineFindus requested a review from zeenix November 30, 2023 18:25
Copy link
Contributor

@zeenix zeenix left a comment

Choose a reason for hiding this comment

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

The default behavior is changing here (which is a pretty big change and I'd like to see it hilighted) and the commit log summary doesn't capture it. It would be best to split the behavior change into a separate commit that comes before this one.

zbus_xmlgen/src/main.rs Outdated Show resolved Hide resolved
@FineFindus FineFindus force-pushed the feat/xmlgen-split branch 2 times, most recently from 32b368d to bbb723e Compare November 30, 2023 19:30
Adds a new `--output` option. If a path is given with it, all the output is written to the file. If `-`, or any other argument, including nothing is given, the generated code is redirected to stdout
If the `--output` argument is omitted, the interfaces are split into individual files.
Copy link
Contributor

@zeenix zeenix left a comment

Choose a reason for hiding this comment

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

As I wrote in the PM:

you did things the other way around. :(
the change of behavior still doesn't get hilighted in the commit history
I suggested that you first change the behavior so output is no longer sent to stdout but to separate interface files, and then add the -o so user can modify (including - for getting the old behavior)
but oh well
I'll just have to try hard and remember when writing the release notes

@zeenix zeenix merged commit 26ad663 into dbus2:main Nov 30, 2023
7 checks passed
@FineFindus FineFindus deleted the feat/xmlgen-split branch November 30, 2023 22:18
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.

2 participants