doc: improve doc of NewConstructor #1391
Conversation
Constructors behave a bit differently under the method selection than other operations but the documentation did not describe this.
|
While at it, maybe also hook up the diff --git a/doc/ref/create.xml b/doc/ref/create.xml
index 224cccaa7..8a82fb7e5 100644
--- a/doc/ref/create.xml
+++ b/doc/ref/create.xml
@@ -1182,6 +1182,7 @@ See also Section <Ref Sect="More About Global Variables"/>.
<#Include Label="DeclareProperty">
<#Include Label="DeclareFilter">
<#Include Label="DeclareOperation">
+<#Include Label="DeclareConstructor">
<#Include Label="DeclareGlobalFunction">
<#Include Label="DeclareGlobalVariable">
<#Include Label="InstallValue"> |
There was a problem hiding this comment.
Thanks, I think it's great this is being addressed. I really would like @stevelinton to comment, though.
Also, see issue #601.
Could you also please clarify what the basis for your text is? Did you analyze the code implementing constructor? Did you perform experiments to confirm your findings? If so, could you perhaps including them, say in an example, as suggested in my detailed feedback comments?
Finally,
| ## <A>args-filts</A><M>[i]</M>. Additionally a constructor expects the first | ||
| ## argument to be a filter to then select a method to construct an object. | ||
| ## of <A>cons</A>. Namely the number of arguments must be equal to the length | ||
| ## of <A>args-filts</A>, and the <M>i</M>-th argument |
There was a problem hiding this comment.
Why this early wrapping? That causes an unnecessary diff (thus making it a little bit harder to review this PR), and I see no good reason for doing it?
| ## lies in the filter <A>args-filts</A><M>[i]</M>. | ||
| ## <Ref Func="InstallMethod"/> must require that | ||
| ## the <M>i</M>-th argument lies in the filter <A>args-filts</A><M>[i]</M> | ||
| ## for <M>i \neq 1</M>. |
There was a problem hiding this comment.
Again, weird re-wrapping -- now I have to carefully check what you really changed :-(
There was a problem hiding this comment.
Ah, I just thought it would look cleaner in the end. Didn't think about the weird-looking diffs it would introduce. I'll keep that in mind in the future, thanks. :)
There was a problem hiding this comment.
Luckily I can use http://www.jefftk.com/icdiff on my local computer to make it better, but the easier it is to already review in the browser, the faster you'll get meaningful feedback
lib/oper.g
Outdated
| ## is chosen, since the rank is computed by taking <M>-1</M> times the ranks | ||
| ## of the involved filters. | ||
| ## <P/> | ||
| ## Imagine wanting to construct a new IsMatrixObject without caring |
There was a problem hiding this comment.
"Imagine" ? So, is this an example? Then I'd start with "For example" to make that clear.
Also, don't you mean IsMatrixObj?
lib/oper.g
Outdated
| ## Imagine wanting to construct a new IsMatrixObject without caring | ||
| ## explicitly in which representation it is created. | ||
| ## &GAP; then chooses a filter whis is <Q>as close</Q> to | ||
| ## IsMatrixObject as possible. |
There was a problem hiding this comment.
This leaves me with more questions. E.g. why wouldn't I just use IsMatrixObj? Why would I only want to be "as close" as possible?
How about giving an actual example? Or perhaps declare a constructor, then install 2-3 methods, and document which of them is used for various (also given) example invocations?
There was a problem hiding this comment.
I think "close" is slightly the wrong word. Method selection in this case will choose a constructor which guarantees to produce an object in IsMatrixObj. All other things being equal it will choose the one that promises the fewest extra filters on the grounds that that is presumably easiest to do.
There was a problem hiding this comment.
Ok, I'll get on that.
fingolfin
added
topic: documentation
|
The corresponding code is |
Codecov Report
@@ Coverage Diff @@
## master #1391 +/- ##
==========================================
- Coverage 62.11% 62% -0.12%
==========================================
Files 1028 1028
Lines 349854 349853 -1
Branches 14223 14223
==========================================
- Hits 217315 216922 -393
- Misses 128970 129345 +375
- Partials 3569 3586 +17
|
olexandr-konovalov
added
the
release notes: not needed
Constructors behave a bit differently under the method selection
than other operations but the documentation did not describe this.