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.

Sign up for GitHub

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

Jump to bottom

Unsetting AllowInvalidUtf8 does nothing #2623

Closed
wants to merge 3 commits into from
Closed

Unsetting AllowInvalidUtf8 does nothing #2623

wants to merge 3 commits into from

Conversation

hosseind88
Copy link
Contributor

Closes #2613

@hosseind88
Copy link
Contributor Author

hosseind88 commented Jul 26, 2021 •
edited

I don't know that I have understood the issue correctly or not, but if yes, I guess I should update CHANGELOG right?

epage
epage reviewed Jul 26, 2021
View reviewed changes
src/build/app/settings.rs
@@ -80,8 +80,6 @@ impl_settings! { AppSettings, AppFlags,
=> Flags::ARGS_NEGATE_SCS,
AllowExternalSubcommands("allowexternalsubcommands")
=> Flags::ALLOW_UNK_SC,
AllowInvalidUtf8("allowinvalidutf8")
=> Flags::UTF8_NONE,
Copy link
Member

Choose a reason for hiding this comment

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

UTF8_NONE would also need to go

@epage
Copy link
Member

epage commented Jul 26, 2021

I guess I should update CHANGELOG right?

Yes, when this was originally changed, the CHANGELOG wasn't updated, so we need to make sure that happens

pksunkara
pksunkara requested changes Jul 27, 2021
View reviewed changes
Copy link
Member

@pksunkara pksunkara left a comment

Choose a reason for hiding this comment

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

What Ed said.

@hosseind88
Copy link
Contributor Author

@epage did I update changelog correctly?

epage
epage approved these changes Jul 29, 2021
View reviewed changes
Copy link
Member

@epage epage left a comment

Choose a reason for hiding this comment

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

LGTM but @pksunkara's word is the one that matters

@pksunkara
Copy link
Member

I think StrictUtf8 should be removed in favor of AllowInvalidUtf8. See #269

@epage
Copy link
Member

epage commented Aug 2, 2021

I think StrictUtf8 should be removed in favor of AllowInvalidUtf8. See #269

Could you clarify how #269 is relevant? The transition from AllowInvalidUtf8 to StrictUtf8 was something kbknapp started in v3, after that issue was closed out.

Granted, my proposal in #751 completely changes all of this (switches from AppSetting::StrictUtf8 to ArgSetting::AllowInvalidUtf8) but I figure we can move forward on this PR now since this is the current state of clap and #751 hasn't had a determination done on it yet of what is the path forward.

@pksunkara
Copy link
Member

The transition from AllowInvalidUtf8 to StrictUtf8 was something kbknapp started in v3

I remember something about that, but couldn't find anything yesterday while searching. Can you please point me to that?

@epage
Copy link
Member

epage commented Aug 2, 2021

I remember something about that, but couldn't find anything yesterday while searching. Can you please point me to that?

I can't find it either. I think my impression came from a misreading of #751 and mixing up spelunking sessions. Interesting enough, it looks like AllowInvalidUtf8 was a no-op for most, if not all, of the v2 lifecycle (meaning unsetting it didn't do anything and setting it didn't override StrictUtf8).

What we need is a determination on #751 so we can know how any of these flags should behave and where.

@pksunkara
Copy link
Member

That sounds about right.

@hosseind88
Copy link
Contributor Author

so do I need to change anything?

@pksunkara
Copy link
Member

#2623 (comment) needs to be done as decided in #751

@epage
Copy link
Member

epage commented Aug 9, 2021

@hosseind88 do you want to carry forward with that work? Just want to coordinate so we don't
both do it.

@pksunkara pksunkara mentioned this pull request Aug 9, 2021
Closed
@hosseind88
Copy link
Contributor Author

@epage you mean that you do it? sorry I have been very busy lately and if you can do it, it would be good

@epage
Copy link
Member

epage commented Aug 10, 2021

@hosseind88 alright, I've gotten started on it.

@pksunkara
Copy link
Member

Closing in favor of #2677

@pksunkara pksunkara closed this Aug 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pksunkara pksunkara pksunkara requested changes

@epage epage epage approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unsetting AllowInvalidUtf8 does nothing
3 participants
@hosseind88 @epage @pksunkara