-
-
Notifications
You must be signed in to change notification settings - Fork 726
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
Turn std.container into a package. #2174
Conversation
Ah, it now compiles. Nice. But I think you lost the commit where you actually got rid of |
I applaud every effort to make commits easier to digest, this generally community has much to learn in that respect. However, in this case, all the "duplicate" commits actually make it harder to see what is going on where to me. Maybe you could merge all the commits that just do the same operation to different files together? A rule of thumb I found helpful: If you can't (be bothered to) write a good commit message for your commit, then it is either too large or too small. |
@monarchdodra It now removes container.d plenty more removals now ;) @klickverbot I've had a go at merging the similar commits. The commit history is still a bit messy. |
I think you need to rebase over head first, or the merge will fail.
I agree the history could be improved, but I don't think it's a problem during the review proper. I think it's fine if you just squash at the end. |
LGTM overall. I do have two comments though:
|
@@ -442,7 +453,13 @@ cov : $(SRC_TO_COMPILE) $(LIB) | |||
$(DMD) -cov=54 -unittest -main -run std\stream.d | |||
$(DMD) -cov=53 -unittest -main -run std\socket.d | |||
$(DMD) -cov=0 -unittest -main -run std\socketstream.d | |||
$(DMD) -cov=88 -unittest -main -run std\container.d | |||
$(DMD) -cov=50 -unittest -main -run std\container\array.d |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did coverage dropped this much? Practically by definition some modules must be covered better.
@monarchdodra Both those changes do make sense to me, so I've pushed them. @DmitryOlshansky Code coverage is now corrected. |
|
||
import core.exception, core.memory, core.stdc.stdlib, core.stdc.string, | ||
std.algorithm, std.conv, std.exception, std.range, | ||
std.traits, std.typecons, std.container.util; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The std.container.util
import should be public? That's what I had in mind anyways.
I've found nothing else to add. There are some imports that can probably still be cleaned-up, but that can be saved for a future pull. Just a final squash, and this gets the LGTM from me. The only thing I'm unsure about is how this packaging business works in terms of documentation...? |
Pinging @jmdavis for an informal review. I believe you were the one that designed "package.d"? One question I have is that I was under the impression that the symbols inside a package were a part of the encapsulating module. However, this creates a sub-module for each package. EG: import std.container.array;
void main()
{
auto arr1 = std.container.Array(); //NOPE
auto arr2 = std.container.array.Array(); //OK
} Did we do something wrong here, or was I simply miss-understood? |
@monarchdodra So, if |
Alright. Well, anybody else want to review this? Seems like a big change to pull without a second opinion. |
Turn std.container into a package. Delete container.d Remove totalcontainer from package. Create std.container.util.d and reference it from other containers. Correct code coverage for containers. Add containers for unit testing. Make std.container.util public from any module. Move around imports (avoid version(unittest)). Remove irrelevant unittests.
Squashed. Let me know if there are any more issues, thanks :) |
// Test issue 5920 | ||
version(unittest) | ||
{ | ||
//@@@BUG4274@@@: This cannot be declared as an inner struct. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bug is fixed. Why is this workaround here then ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably because no-one took the time to fix this specific unittest ;)
I think we should be reviewing the packaging, rather than -re-reviewing the totality of container.d
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am reviewing packaging, namely I'm diffing it against my copy of container.d to see if there were any significant changes other then splitting. In git HEAD this was already fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, really? Dang. I didn't realize, sorry. @damianday, could you make sure no work has been lost compared to the latest head?
At least for me generated html is strangely showing only std.container without any submodules, more then that it fails to open. Can anyone double check what the docs look like? |
Lastly I'd note that totalcontainer.d definitely doesn't deserve its own module being an empty skeleton for documentation purposes. |
@monarchdodra All modules now synced with git head, all diffs resolved. @DmitryOlshansky What to do about totalcontainer.d? Perhaps rename to totalcontainer.doc and remove from makefiles etc? |
@damianday I'd merge it with package as this is where general documentation on containers goes. P.S. Anther thing is that you need to squash these commits into one. |
@DmitryOlshansky merged documentation into package and squashed. |
Needs rebase, it's DList which was modified, btw. Let's get this rebased and pulled quick, so it doesn't desync again. |
@monarchdodra re-synced and all green. |
Is the fact that it's actually re-packaged documented anywhere? I can't see it it. Anywhoo, looks "good enough to merge". Anybody see anything blocking? |
It would be cool to see how final dpl docs look like after this, last time I tried building docs locally of this branch it wasn't good. Otherwise LGTM |
Auto-merge toggled on |
Turn std.container into a package.
Each container has been put into its own module.
Unit tests have been moved to the appropriate places.
Makefiles have been updated.
I've tried to keep the commits small to make reviewing simpler.