Skip to content

Add utility to convert OsStr to CString #120

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

Closed
wants to merge 6 commits into from
Closed

Add utility to convert OsStr to CString #120

wants to merge 6 commits into from

Conversation

flub
Copy link
Contributor

@flub flub commented Jun 3, 2019

Several iterations later I think this is a fairly reasonable style and and api for this. Let me know what you think.

@flub flub requested a review from dignifiedquire June 3, 2019 20:55
@flub
Copy link
Contributor Author

flub commented Jun 3, 2019

Btw, I didn't know how to produce an OsString with invalid unicode in for a test so didn't include such a test. If you know how to do that I'd be happy to add such a test.

@dignifiedquire
Copy link
Collaborator

btw, I didn't know how to produce an OsString with invalid unicode in for a test

Did you try casting raw bytes to one?

@flub
Copy link
Contributor Author

flub commented Jun 3, 2019

Tried this:

let some_str = std::ffi::OsString::from(b"foo\xC0bar");
     |                        ^^^^^^^^^^^^^^^^^^^^^^^^ the trait `std::convert::AsRef<std::ffi::OsStr>` is not implemented for `[u8; 7]`

but it doesn't like that as you can see, did you mean something else with casting?

@flub
Copy link
Contributor Author

flub commented Jun 4, 2019

Any way to tell circleci to not run the upload_doc_wheels job for a PR from a different repo? My quick hack would be to just change the early exit in the script to 0 so it succeeds but I'm not sure that's desired. /cc @hpk42

@hpk42
Copy link
Contributor

hpk42 commented Jun 5, 2019

@flub i think one could skip the whole upload_test_wheels thing if "DEVPI_LOGIN" is not defined. i am doing my branches in-repo so the problem didn't occur to me yet.

@flub
Copy link
Contributor Author

flub commented Jun 5, 2019

@hpk42 so you're happy with doing this in the script rather that the circleci config?

@hpk42
Copy link
Contributor

hpk42 commented Jun 5, 2019

@flub no preference, was just pointing out a possibility. i guess it'd be better to only run the script/step on the merged branch, not the branch itself.

@flub
Copy link
Contributor Author

flub commented Jun 5, 2019

ping, i reckon this is ready

@flub
Copy link
Contributor Author

flub commented Jun 5, 2019

Oh, maybe the trait should use AsRef<OsStr> and than I'd not need the impl for Path. I think that's nicer. I'll refactor that way.

@flub
Copy link
Contributor Author

flub commented Jun 5, 2019

Scratch that, that doesn't seem to work. I'll stick with this as is.

@hpk42
Copy link
Contributor

hpk42 commented Jun 6, 2019

hum, there is no code yet using these utils or am i missing something? If so, wouldn't it make sense to have a few files use the utils and see that it actually fits?

@flub
Copy link
Contributor Author

flub commented Jun 6, 2019

Perhaps, I'm actually factoring this out of a much larger change to make reviewing more sane. But it's true that said change is not in a PR yet. Is there a way to chain PRs in github so I can both these at the same time? If so I'm happy to leave this lying around till the PR that uses it is ready too.

@ralphtheninja
Copy link
Collaborator

Is there a way to chain PRs in github so I can both these at the same time?

Yes there is. Just make a new branch based on top of this one and make a PR with that. When merging the second PR, this PR will be closed automatically. But you might as well just add some commits on top of this branch.

Floris Bruynooghe added 6 commits June 6, 2019 23:36
This is approach seems acceptable in the context of deltachat, it
should work correctly on unix and on Windows requires paths to be
valid utf-8.
- Adds an OsStrExt impl for Path directly, a little more convenience.

- Fix the windows code to actually use the right function name.  Test
  the impl function on unix too since that was the point of having it
  implemented in a separate function to begin with.

- Improve the docs, do hyperlinks a bit better.
Having your compiler in the cloud is just painful.
When PRs are made from forks the passwords are unavailable.  We don't
want CI to fail because of this.
This means any type with implements this trait will get this
implementation, thus covering both OsStr and Path instead of having
duplicate implementations for those like before.
Copy link
Collaborator

@dignifiedquire dignifiedquire left a comment

Choose a reason for hiding this comment

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

just some formatting issues, other than looks good to me

@ralphtheninja
Copy link
Collaborator

Made a new PR with format changes here #136

@flub flub deleted the os_str_to_c_string branch June 8, 2019 09:17
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.

4 participants