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

Change all event and Attributes constructors to accept strings #428

Merged
merged 9 commits into from
Jul 24, 2022

Conversation

Mingun
Copy link
Collaborator

@Mingun Mingun commented Jul 18, 2022

This PR is extended version of #424. It changes all events' constructors to accept &str / String and removes all public constructors that accepts byte arrays. Internally these constructors are still used, so all tests still passed. On now we have our hands untied in changing the internal representation of events (well, almost, BytesStart::set_name still accepts &[u8]).

I did not include name changes in this PR, because I want to make more radical renames instead of just removing _str suffix. I'll made corresponding PR tomorrow.

@Mingun Mingun added enhancement encoding Issues related to support of various encodings of the XML documents labels Jul 18, 2022
@Mingun Mingun requested a review from dralley July 18, 2022 19:16
@dralley
Copy link
Collaborator

dralley commented Jul 23, 2022

Approved: but please allow #425 to be merged first

@Mingun
Copy link
Collaborator Author

Mingun commented Jul 23, 2022

Actually, I would to merge #425 as late as possible, because I want to play with the possible improvements next week. I see that here we have a consensus, so it seems that this could be merged sooner

@dralley
Copy link
Collaborator

dralley commented Jul 23, 2022

I would prefer for it to be merged as soon as possible, for the same reason : / Everything which touches those files is effectively blocked on that PR, and the alternative is forcing 999eagle to rebase it over and over and over again, which doesn't feel like a great way to encourage further contributions. And they're not minor rebases but terribly painful ones.

I hear your concerns over the duplication of code, but I see a clear path towards addressing it and I've already made a lot of progress. I kinda feel like we're just piling up more and more merge conflicts for no particularly good reason.

@dralley
Copy link
Collaborator

dralley commented Jul 24, 2022

Well, the crux of the issue is the file split (which makes practically every change a breaking one), and your namespace PR includes it, so for the sake of making at least some forwards progress on this issue, we can go with that one first....

@dralley dralley merged commit e738b68 into tafia:master Jul 24, 2022
@Mingun Mingun deleted the encoding branch July 24, 2022 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
encoding Issues related to support of various encodings of the XML documents enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants