-
Notifications
You must be signed in to change notification settings - Fork 287
Modules rewrite #4570
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
base: main
Are you sure you want to change the base?
Modules rewrite #4570
Conversation
|
I guess my main point is that it's better than it was before :) . |
coke
left a comment
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.
Is there a curl option to hide the output instead?
coke
left a comment
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.
RAKULIB should be . (current directory's META6.json), not lib
|
This seems to be a good start. I absolutely agree the modules part needed restructuring. From the site building point of view, it does not matter which directory the files are placed, so long as the meta date at the top of the file (in the line with Richard |
|
|
@wayland Ok understood that the main work currently is about regrouping, and that's definitely important. I do mean a page on documenting a module. I agree more needs writing - its a TODO of mine. |
|
Hi! I've made the following updates:
HTH, |
|
|
||
| However, this is I<not> because it returns an L<C<IO::Path>|/type/IO::Path>, but because it | ||
| shares many method with it: C<Str, gist, raku, absolute, is-absolute, | ||
| However, this was found to be problematic due to the fact that the path can |
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'm not sure it makes sense to talk about what why things are implemented the way they are in the docs e.g. "was found to be problematic". Someone looking at the docs presumably wants to know how to use something and not necessarily the history or implementation details.
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.
OK, I've rewritten this section so that it:
- Keeps most of the old material (but updates the deprecation list
- Adds a paragraph on what not to do (which was what I was trying to achieve), but without listing the deprecated methods
Would very much appreciate your feedback, since I seem to get this wrong on the regular.
Thanks!
| get compiled into the library at compile time, which means that it can end | ||
| up pointing to the wrong place at runtime. Thus, many of the methods that | ||
| were shared with L<C<IO::Path>|/type/IO::Path> are now deprecated; this | ||
| includes C<Str, gist, raku, absolute, is-absolute, |
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.
If Str or gist is giving a deprecation message then that is a bug
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 was looking at https://github.com/rakudo/rakudo/blob/b527cefcb29789120f32d5e4003913e53c47476f/src/core.c/Distribution/Resource.rakumod#L50 when compiling this list; was that incorrect?
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 meant .raku or .gist aren’t deprecated
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! Right. Well, I've completely left them out this time around because
- I wasn't sure of their status
- They're not specific to mirroring IO::Path, they're on just about everything, so I figured they didn't need a mention.
HTH,
| relative, is-relative, parts, volume, dirname, basename, extension, open, | ||
| resolve, slurp, lines, comb, split, words, copy>; above we use C<.lines>, for | ||
| instance. | ||
| resolve, slurp, lines, comb, split, words, copy>. |
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.
This list is not accurate. You can see the deprecations explicitly defined in the source. Notably slurp, lines, and other methods that don't return a string representation of a path or path part.
I also think listing all the deprecated methods is rather distracting, and that listing the methods that are actually allowed (similar to the original description) makes the most sense.
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.
OK, I'll be rewriting this section in line with your recommendations.
Just my notes on the old version of the list as to what's still current and what isn't. I'm mainly posting it here for feedback on .gist and .raku (given that .Str is deprecated).
- Deprecated: Str, absolute, is-absolute, resolve
- Not sure: gist, raku
- Still good: slurp, lines, comb, split, words, copy
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.
Str is deprecated?
|
Also regularised use of subkind across the new pages (to "Modules"). |
|
Hi @ugexe , are you happy with this now? |
|
Hi @ugexe , let me ask a different question: Can you have a look at your previous comments, and mark them "resolved" if you're satisfied with my changes? Thanks! |
| names in all distributions available. | ||
|
|
||
| Given this arrangement, the B<rakudo> compiler, or a package manager such as | ||
| B<zef>, can obtain all the information needed about each C<compunit> from a |
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 think this PR is incorrectly using the term compunit in a lot of places. See this doc and note there are separate definitions for compunit and module. Many places this references compunit I would have expected module.
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.
Thanks! Definitely the best explanation I've seen of the terms. I have:
- Redone the doc to use "module" instead of "compunit"
- Added some of the definitions from your link to the documentation.
| Currently, there are three ways to distribute a module. No matter which method | ||
| you choose, your module will be indexed on the L<raku.land|https://raku.land> | ||
| and L<raku.land|https://raku.land> websites. The three module | ||
| distribution networks, or ecosystems, are: |
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'm not sure it is useful to mention anything other than the zef ecosystem, particularly in the context of a user "choosing" one. See https://github.com/Raku/problem-solving/blob/main/solutions/meta/sunsetting-p6c-cpan.md
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.
OK, I was just copy-pasting what was elsewhere in the docs. I've removed references to CPAN and P6C.
Thanks!
|
Hi @ugexe , I've addressed your points, thanks very much. Any more objections? Thanks |
|
Hey, sorry I got very derailed in July (!) and lost track of a bunch of stuff. What’s left to get this merged? |
|
Please also (sorry!) resolve the conflicts. Very sorry. Once that’s passed, would love to merge in next few days. Thanks! |
coke
left a comment
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.
In general, content all looks great, I just have a few minor requests just added.
| @@ -0,0 +1,75 @@ | |||
| =begin pod :kind("Language") :subkind("Modules") :category("tutorial") | |||
|
|
|||
| =TITLE Distributions: An Introduction | |||
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.
As you know, xt/headings.rakutest is failing on most of these changes: I added some comments to the rakudoc overview on that test file on main that should help pass. This one should be
Distributions: an introduction
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.
When you resubmit, you’ll see we’ve added some CI tests - please make sure those pass.
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 didn't see any CI tests, but I made sure all the tests passed on my local.
… On the downside, the output is uglier. On the upside, it doesn't hang.
- Removed references to P6C and CPAN - Changed compunit to module where appropriate
ca92f0d to
0100739
Compare
|
Hi @coke How now? :) |
coke
left a comment
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.
partial review, didn’t go through everything.
| obligation to use it. | ||
|
|
||
| Create a project directory named after your module. For | ||
| example, if your module is C<Vortex::TotalPerspective>, then create a |
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.
Accidental indent?
| the C<lib/> directory, then it is possible to use | ||
|
|
||
| =for code :lang<shell> | ||
| prove6 -Ilib |
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 would consider removing this whole section, or perhaps replace with just one sentence explanation that explains why the previous is better.
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.
This is pre-existing (just moving stuff around).
| There have been three ways to distribute a module, but the CPAN and P6C ways | ||
| have been deprecated; please use the zef/fez method instead. However, no | ||
| matter the method, all modules will be indexed on the L<raku.land|https://raku.land> website. | ||
|
|
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.
Let’s not lead with history, but with the current standard; leave the history as the last (small) entry on the page.
| If you want to use the I<zef> ecosystem then you need to register your username | ||
| using I<fez>. | ||
|
|
||
| =item Install fez if you haven't done so already |
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.
C<fez>
|
|
||
| You can combine C<EXPORT> with type captures for interesting effect. This | ||
| example creates a C<?> postfix which will only work on L<C<Cool>|/type/Cool>s; | ||
| please note that, by using C<$_> as an argument, we don't need to use a |
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.
Do we need to introduce the topic topic here?
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.
This is pre-existing (just moving stuff around).
|
@coke do you mind if we keep this PR focussed on the existing changes, and not rewriting things that are just moving things around? What I'm worried about is that this will again get pushed on to the back burner by one of us, and then there will be more moerge conflicts to overcome, and it could take ... a long time. Really all I'm asking is permission to say "Let's not worry about that now" if it's sub-optimal, but a) pre-existing, and b) not actually wrong. |
|
Can you describe which stuff is new and which is just moved? That would help with the review. |
The problem
The module/package documentation was suboptimally organised, as per #3714
Solution provided
Organised documentation less suboptimally.
Things to note:
I'm expecting feedback on things I need to change.
Hope this helps.