doc icon indicating copy to clipboard operation
doc copied to clipboard

Largely rework the Object Tutorial 2nd try

Open patrickbkr opened this issue 3 years ago • 13 comments

This is a reworked version of https://github.com/Raku/doc/pull/3794. The changes are largely the same, for the largest part only pulled apart into many separate commits.

With the advent of is built all the common use-cases of object construction are now covered by our attribute syntax. This means that the need to fall back to using BUILD has lessened enormously. Given that BUILD has a number of behaviours that are non-obvious and require knowing and understanding them, it makes sense to consider BUILD the last resort mechanism to use when all else fails and recommend using automatic attribute initialization and TWEAK for everything else. Communicating this clearly in our object tutorial is very valuable, it shows newcomers the sane default first. We do not want to send them off in the wrong direction as BUILD has proven to be a stumbling block for new users numerous times.

List of Changes:

  • Try to be more structured by reordering things.
  • Add more headings for easier navigation.
  • Explain is built.
  • Rework examples to not use BUILD anymore.
  • Remove wrong explanation of the Task example stating one would need a BUILD when custom constructors are used.
  • Remove in-depth explanation of BUILD. Add respective documentation for TWEAK. BUILD is still explained in detail in the Object Reference.
  • State that BUILD is only intended for advanced use-cases.
  • Remove Hero example which is made obsolete by is built.
  • Simplify the Str-with-ID example and remove bits promoting making TWEAK a method instead of a submethod.
  • Remove the second duplicate Str-with-ID example.
  • Rename Static Fields? heading to Class variables
  • Fix the our / my example to show what Raku actually does.

patrickbkr avatar Apr 04 '22 16:04 patrickbkr

Couple of nits:

  1. There's a conflict you will need to solve.
  2. I haven't seen any reference to the release where is built was introduced. Maybe it would be convenient you worked it into the first time you mention it.

Also, please bear in mind that this is a long PR that will need some time to check out as it deserves. One of the things that woudl help is that you commented in the blocks of code that you've moved that you have effectively done so and where; otherwise, I'd have to work out if something is a deletion or a rearrangement.

JJ avatar Apr 04 '22 16:04 JJ

Couple of nits:

1. There's a conflict you will need to solve.

Right. I just continued where I left of the last time (quite some time ago) and forgot to rebase. Will do.

2. I haven't seen any reference to the release where `is built` was introduced. Maybe it would be convenient you worked it into the first time you mention it.

Will do.

Also, please bear in mind that this is a long PR that will need some time to check out as it deserves. One of the things that would help is that you commented in the blocks of code that you've moved that you have effectively done so and where; otherwise, I'd have to work out if something is a deletion or a rearrangement.

I tried to be rather atomic in the individual commits (not 100%, but pretty much). If a move isn't in a commit with a message containing the word "move", then I missed it. Or do you mean I should comment somewhere else than the commit message?

patrickbkr avatar Apr 04 '22 19:04 patrickbkr

Right. Commit messages are not available when reviewing. Just comment inline as if you would be commenting someone else's PR.

JJ avatar Apr 04 '22 20:04 JJ

Right. Commit messages are not available when reviewing. Just comment inline as if you would be commenting someone else's PR.

Done.

patrickbkr avatar Apr 04 '22 21:04 patrickbkr

Please check the error in CI. Apparently, there's some mismatched markup in the pod. Comma should help you with that.

JJ avatar Apr 08 '22 15:04 JJ

Also, #3794 has a number of comments. It's not clear if you have addressed them here, or simply started from scratch and those comments do not apply.

JJ avatar Apr 08 '22 18:04 JJ

Also, #3794 has a number of comments. It's not clear if you have addressed them here, or simply started from scratch and those comments do not apply.

I have just looked at all the comments in the previous PR. I believe I have addressed all by either not doing the opposed change or explaining it explicitly in a separate commit message.

patrickbkr avatar Apr 22 '22 11:04 patrickbkr

Please check the error in CI. Apparently, there's some mismatched markup in the pod. Comma should help you with that.

I believe I have fixed it. Let's see what the CI says.

patrickbkr avatar Apr 22 '22 11:04 patrickbkr

Which ones do you refer to? All of them? Those that have been removed?

I don't understand this comment.

patrickbkr avatar Apr 22 '22 12:04 patrickbkr

@JJ I have rebasted to master, the CI is green and I have worked through all the comments. I'm unaware of anything I can do to push this further. I'm now waiting on your input.

patrickbkr avatar Apr 22 '22 14:04 patrickbkr

Sorry, I'm no longer reviewing or merging PRs to the main branch. Feel free to rebase this to the old-docs branch, or else wait for it to be reviewed and merged to that branch; I'll incorporate it to that branch if it's suitable for it. Thanks for understanding it.

JJ avatar Apr 25 '22 11:04 JJ

BTW, I'm still seeing "this branch has conflicts".

Question: when merging, do you want to preserve the commit history here or is a squash OK?

coke avatar May 09 '22 16:05 coke

That conflict is caused by bitrot since my last push. I'll rebase this on master again. I'm ok with squashing and in this case I don't have a strong preference on whether to merge or squash. (Ironically I put a huge amount of work into refactoring the original PR into separate commits as I was asked to do so for easier reviewing.)

patrickbkr avatar May 09 '22 16:05 patrickbkr

I realized I forgot to push the rebase I did some months ago. So finally it's pushed. @coke Can you have another look?

patrickbkr avatar Oct 07 '22 08:10 patrickbkr

Thanks, will do!

coke avatar Oct 07 '22 14:10 coke

Ping @coke. A month has passed.

patrickbkr avatar Nov 07 '22 08:11 patrickbkr

Thank you, will review this weekend.

coke avatar Nov 09 '22 15:11 coke

Thank you, will review this weekend.

Done; one conversation left about the mnemonic; the header items we can fix "in post".

coke avatar Nov 14 '22 03:11 coke