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

Iterating over Ini yields "Section" type instead of "SectionIter" (2.0 - breaking change) #42

Merged
merged 4 commits into from
Nov 16, 2021

Conversation

dralley
Copy link
Contributor

@dralley dralley commented Aug 12, 2021

From the previous discussion here: #35

I have no expectations about this being merged quickly or anything since it's a change for 2.0, I just wanted to put this out early for review at your leisure.

I expect that the API still needs some improvement and I might have missed some things in the docs. I'll come back to it eventually.

src/lib.rs Show resolved Hide resolved
@FreeCX FreeCX requested a review from citrux August 18, 2021 14:43
@FreeCX
Copy link
Member

FreeCX commented Aug 18, 2021

Hi @dralley.
Thank you for pull request!
We'll reviewing it this weekend.

fn into_iter(self) -> Self::IntoIter {
IntoIter { base: self.base, keys_iterator: self.keys.into_iter() }
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The compiler complained about conflicting implementations

@FreeCX FreeCX self-requested a review August 22, 2021 15:15
@FreeCX
Copy link
Member

FreeCX commented Aug 22, 2021

Hi @dralley.

Now we do not plan to public (in docs) any part of ordered_hashmap.rs.
That why we wrap it in Ini and iterator structs.

Current documentation
image

Pull request
image

cc @citrux

@dralley
Copy link
Contributor Author

dralley commented Aug 22, 2021

I figured as much, I can fix it without much trouble.

Copy link
Member

@FreeCX FreeCX left a comment

Choose a reason for hiding this comment

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

@dralley
Copy link
Contributor Author

dralley commented Aug 22, 2021

The general principle is fine though, yes?

@FreeCX
Copy link
Member

FreeCX commented Aug 23, 2021

I think so, if we keep the parsing capabilities of get.

@dralley dralley force-pushed the master branch 2 times, most recently from d6360f7 to cbf88f3 Compare September 3, 2021 15:42
@dralley
Copy link
Contributor Author

dralley commented Sep 3, 2021

@FreeCX Apart from missing documentation and maybe some additional tests / cleanup, how does this look

@FreeCX
Copy link
Member

FreeCX commented Sep 3, 2021

@FreeCX Apart from missing documentation and maybe some additional tests / cleanup, how does this look

I think pretty good

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