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

add option for no extended chars #393

Merged
merged 1 commit into from
Dec 26, 2019
Merged

add option for no extended chars #393

merged 1 commit into from
Dec 26, 2019

Conversation

adamralph
Copy link
Owner

@adamralph adamralph commented Dec 26, 2019

-E, --no-extended-chars    Disable extended characters

image

@adamralph adamralph added the enhancement New feature or request label Dec 26, 2019
@adamralph adamralph added this to the 3.2.0 milestone Dec 26, 2019
@adamralph
Copy link
Owner Author

@ygel I've released this in 3.2.0-alpha.2. Would you like to give it a try?

@adamralph adamralph mentioned this pull request Dec 26, 2019
18 tasks
@adamralph
Copy link
Owner Author

ping @ygel ☝️

@ghost
Copy link

ghost commented Jan 13, 2020

Hi @adamralph, thanks for adding this. Is there a way to specify this option without modifying command-line? Would be nice to have it as an arg for calling RunWithoutExitingAsync and similar, say:

await bullseyeTargets.RunWithoutExitingAsync(
    productBuildTargets,
    logPrefix: "Foo",
    noExtendedChars: true);

This change is confirmed good for my use case.

@adamralph
Copy link
Owner Author

Is there a way to specify this option without modifying command-line?

@ygel I guess you could construct an instance of Options and explicitly set NoExtendedChars to true, but you'd also have to deal with parsing string[] args into IEnumerable<(string, bool)> to pass into the Options constuctor.

Generally, I'm not sure it's a great idea to set this option in code, since it's use is very much dependent on the environment that the build targets are being run in. E.g. when running locally, extended chars are fine, but when running in some CI environment, they aren't. So it's generally better to control this option via the command line.

@ghost
Copy link

ghost commented Jan 20, 2020

Hello, @adamralph, I understand. Your solution covers intended usage from Bullseye POV. At this point I opted to not not expose the runner's command line handling to the user, this might change.

Going forward, I think it would be beneficial to have both:

  • Ability to read/modify Options instance Bullseye parsed
  • Simplify creation of a fresh Options instance and pass it to the c-tor.

Thank you!

@adamralph
Copy link
Owner Author

Ability to read/modify Options instance Bullseye parsed

@ygel you can do this today, but the method is officially "internal", meaning a breaking change could be introduced in a minor:

var (targetNames, options) = Bullseye.Internal.StringExtensions.Parse(args);
options.NoExtendedChars = true;

I'll consider moving the method to the public namespace.


Simplify creation of a fresh Options instance and pass it to the c-tor.

I'm not sure what you mean by "pass it to the c-tor", but creating an instance of Options can't get much simpler than it is today:

var options = new Options { NoExtendedChars = true };

@ghost
Copy link

ghost commented Jan 20, 2020

@ygel you can do this today, but the method is officially "internal", meaning a breaking change could be introduced in a minor:

var (targetNames, options) = Bullseye.Internal.StringExtensions.Parse(args);
options.NoExtendedChars = true;

I'll consider moving the method to the public namespace.

Thank you for looking at it! There's no rush on this at all however, I'm at a stable state for feature freeze and it can wait.

I'm not sure what you mean by "pass it to the c-tor",

Oh I haven't actually looked at the code for this and was reflecting on your comment from above about possible non-triviality of hand-parsing options. To clarify what I meant by "simplify" is to have Bullseye parse command line and then give the caller an Options object to modify. Having typed all of this out, I now realize that this request is a subset of the one above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant