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

Fix #83 and #30. #84

Merged
merged 1 commit into from
Aug 8, 2016
Merged

Conversation

ciechowoj
Copy link
Contributor

@ciechowoj ciechowoj commented Aug 2, 2016

Fix #83: New multiline translation.
Fix #30: Single space inserted after function names.

I know that there was a discussion (#30) about adding formatting features to dstep with some negative arguments, but it doesn't cost much to add them, they can save some time for some people and apparently there is a need for them from some users (at least two at the moment).

"dont-reduce-aliases", "Disable reduction of primitive type aliases.", &config.dontReduceAliases);
"dont-reduce-aliases", "Disable reduction of primitive type aliases.", &config.dontReduceAliases,
"single-line-function-headers", "Do not break function headers to multiple lines.", &config.singleLineFunctionHeaders,
"no-space-after-function-name", "Do not put a space after a function name.", &config.noSpaceAfterFunctionName);
Copy link
Owner

Choose a reason for hiding this comment

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

Would it be possible to have both a positive and negative argument, like --space-after-function-name and --no-space-after-function-name. This is quite common for Posix commands, see git add --help where there is a --[no-]ignore-removal command, for an example.

Copy link
Contributor Author

@ciechowoj ciechowoj Aug 8, 2016

Choose a reason for hiding this comment

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

It cannot be done automatically with getopt. The only way I see is to add a negation of given option to Configuration and check the constraints in code. Something like:

 "no-space-after-function-name", "Do not put a space after a function name.", &config.noSpaceAfterFunctionName,
 "space-after-function-name", "Put a space after a function name.", &config.spaceAfterFunctionName);

/+ ... +/

if (config.noSpaceAfterFunctionName && config.spaceAfterFunctionName) {
    // very bad
}

Copy link
Owner

Choose a reason for hiding this comment

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

Then we'll leave it as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I rethought the matter and getopt seems to support assigning of a true and false to the CLI switch. Maybe it would be better to resign from no- prefixed switches and require users to specify the desirable value explicitly instead. E.g.

--reduce-aliases=false
--comments=true

I just want to point out that possibility.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, I think that looks better than all the dont... flags.

@jacob-carlborg
Copy link
Owner

Please add an entry to the changelog as well.

Fix jacob-carlborg#30: Single space inserted after function names.
@ciechowoj
Copy link
Contributor Author

Please add an entry to the changelog as well.

Done.

@jacob-carlborg
Copy link
Owner

👌

@jacob-carlborg jacob-carlborg merged commit 4e63beb into jacob-carlborg:master Aug 8, 2016
@ciechowoj
Copy link
Contributor Author

Thanks.

@ciechowoj ciechowoj deleted the issue_83 branch August 8, 2016 21:14
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

2 participants