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
Incorrect generation of "constructor" tags for modules #7596
Comments
Comment author: @Octachron I agree that the "constructor" class is quite misleading since it is in fact applied to any capitalized identifier, module and constructors alike. There are few places where ocamldoc could use more accurate classes (in particular for references), but in general ocamldoc does not have enough information to distinguish the two: for instance in the standard library [String] can be either the module "String" or the constructor "Arg.String". |
Comment author: theindigamer So I checked the manual and it does have a way to provide annotations (http://caml.inria.fr/pub/docs/manual-ocaml/ocamldoc.html#sec333) as mentioned in section 15.2.4.2. It is just that Pervasives does not use this. It makes sense though: manually annotating all references would be very time-consuming. If Ocamldoc were to use a deductive system to guess the type of reference that you are trying to make, that would solve the issue for the use case when there are no name conflicts. I suspect that this case happens at least as frequently as the conflicting case, if not more. So for this sub-case, the documentation author gets the benefit of correct semantics without any additional work. Of course, this doesn't solve the problem for the "String" vs "Arg.String" case which you pointed out -- I think one has to use manual annotation in that case as in section 15.2.4.2. However, it would be very helpful if Ocamldoc emitted something like: Warning [#warning-code] : so that the author knows that their documentation is buggy. |
Comment author: @gasche I agree that this feature sounds useful (especially with the suggested syntax that would help make the feature better-known), although deploying it in practice may be delicate if everybody's documentation warns too often. |
Comment author: theindigamer
Good point. I suspect that the current I think there are a couple of options: Option 1: If the exact look of documentation is to be kept unchanged, then make a This way people who were using things with default settings need not be bothered. Only people using -css-style and having ambiguous naming will need to supply this additional flag (I suggest adding a warning if -css-style is supplied && there is at least one ambiguity && -W-ambiguous is not supplied). Option 2: If you want the default documentation to have different colours, then update So option 1 means people can blissfully ignore stuff and proceed as usual, so long as they stick to the default css, which I think covers a majority of use cases. Option 2 would probably be a much ruder shock to authors of large packages. My hunch is to go with option 1 as: (a) a large portion of the userbase doesn't get annoyed. |
Comment author: @Octachron @theindigamer, I fear that there may be a confusion here, the namespace indication for reference is only used to find the target of the cross-reference, it has currently no influence on the "constructor" tag. Internally ocamldoc interprets "{!namespace:Name}" as "{!namespace:[Name]}" and therefore "{!module:String}" stil generates a "constructor" span tag for "String". Moreover, in absence of namespace annotation, ocamldoc already tries to find a corresponding target in all namespaces. On this point, I agree that having a warning when the name is ambiguous would be useful (even moreso since ocamldoc does not enforce any priority order beween namespaces). |
Comment author: theindigamer @Octachron, got it, I was mixing up two separate things in my head. So to summarize, there's two uncoupled things needed to be done, right?
Initially, I (incorrectly) thought that the latter was an issue because we did not know the namespaces with enough resolution. So we do actually know them (apart from the ambiguous case). |
Comment author: @Octachron I believe that warnings should always be optional; at most, it could make sense to enable it by default if the specificity of the warning is high enough. |
Comment author: theindigamer Oops, I mistranslated "opt-in or opt-out" to "optional or not" from my brain to the keyboard. Warnings have to be optional, of course. |
This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc. |
Original bug ID: 7596
Reporter: theindigamer
Assigned to: @Octachron
Status: assigned (set by @Octachron on 2017-07-22T13:20:48Z)
Resolution: open
Priority: low
Severity: minor
Platform: Linux
OS: Ubuntu
OS Version: 16.04
Version: 4.04.0
Category: ocamldoc
Monitored by: @gasche
Bug description
Ocamldoc seems to generate misleading constructor tags for modules. While I've seen this locally, the simplest way to "reproduce" this is to inspect the source for the Pervasives documentation:
http://caml.inria.fr/pub/docs/manual-ocaml/libref/Pervasives.html
For example, on inspecting the source for the line "More string operations are provided in module String." in the section String operations (line 767 as seen on 2017/07/22):
More string operations are provided in module
String
.I don't think the constructor tag should be there in the first place -- as it mixes modules and value constructors under the same tag.
(Sorry if this has been reported before/solved already, I checked the bug tracker once and I couldn't find anything discussing this point.)
Unrelated: it seems the CSS is wonky for the Parameters table here -- http://caml.inria.fr/pub/docs/manual-ocaml/libref/Set.Make.html -- from what I understand, the
<code>
tag inside the table (for the first entry) causes the text to shrink and thevalign
attribute causes it to hit the cell ceiling. As far as I can tell, this can be fixed by re-assertingfont-size : blah
inside tables for the CSS.(Tested on Firefox 54.0)
Steps to reproduce
See description.
The text was updated successfully, but these errors were encountered: