Skip to content

Conversation

@fingolfin
Copy link
Member

Fixes #66 and generally makes AutoDoc parsing a bit stricter.

Now that I think about it: a few people may have abused the fact that GAP parses commented out declarations... Alas, we never documented this, and I'd argue that we never should have parsed those. People who abused it this way should file issues with us telling us about their use cases, so that we can support it properly.

## searched cause problems otherwise.
@DONT_SCAN_NEXT_LINE := function()
ReadLineWithLineCount( filestream );
end,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Being able to get rid of @DONT_SCAN_NEXT_LINE is a nice side benefit, IMHO.

@fingolfin fingolfin force-pushed the mh/fix-commented-out-decl branch from 2fed9e0 to 79f6397 Compare July 17, 2019 09:22
@codecov
Copy link

codecov bot commented Jul 17, 2019

Codecov Report

Merging #224 into master will increase coverage by 0.03%.
The diff coverage is 90.62%.

@@            Coverage Diff             @@
##           master     #224      +/-   ##
==========================================
+ Coverage   74.95%   74.98%   +0.03%     
==========================================
  Files          13       13              
  Lines        2336     2347      +11     
==========================================
+ Hits         1751     1760       +9     
- Misses        585      587       +2
Impacted Files Coverage Δ
makedoc.g 94.73% <66.66%> (-5.27%) ⬇️
gap/Parser.gi 63.14% <93.1%> (+0.32%) ⬆️

1 similar comment
@codecov
Copy link

codecov bot commented Jul 17, 2019

Codecov Report

Merging #224 into master will increase coverage by 0.03%.
The diff coverage is 90.62%.

@@            Coverage Diff             @@
##           master     #224      +/-   ##
==========================================
+ Coverage   74.95%   74.98%   +0.03%     
==========================================
  Files          13       13              
  Lines        2336     2347      +11     
==========================================
+ Hits         1751     1760       +9     
- Misses        585      587       +2
Impacted Files Coverage Δ
makedoc.g 94.73% <66.66%> (-5.27%) ⬇️
gap/Parser.gi 63.14% <93.1%> (+0.32%) ⬆️

@zickgraf
Copy link
Contributor

We use the fact that commented operations are parsed to add documentation for operations which are already installed for more general filters, see e.g. https://github.com/homalg-project/FinSetsForCAP/blob/25c0b9b02752de6d468d19fd79d2d13e16dbb41d/gap/FinSetsForCAP.gd#L120

It would be nice to have a replacement for this.

@fingolfin
Copy link
Member Author

Since people are using this, this PR can't be merged as-is. I propose the following strategy:

  1. Add an alternative mechanism for adding ManSections without following DeclareFOOBAR commands. E.g. by adding a @ManSection TYPE NAME command, used as @ManSection Prop IsFOO; other suggestions are also welcome!
  2. Add code which detects when AutoDoc "sees" a commented out operation/property/... as in #DeclareProperty("IsFoo", IsBar); and documents it. Then have it print out a deprecation warning.
  3. Make an AutoDoc release with those two, and wait. Optionally, also run as many packages makedoc.g file and watch for the deprecation warning, then submit issues / PRs to any affected packages
  4. Once all packages switch to the new approach, we can finally remove support for this mis-feature, i.e., merge this PR in its current form.

@zickgraf
Copy link
Contributor

I have two other ideas for an alternative mechanism (I'm not suggesting they are better than you're approach, just two random ideas):

  1. Do not parse DeclareProperty after a regular comment #, but parse it after an AutoDoc comment #!.
  2. Introduce @DeclareProperty which is parsed exactly as DeclareProperty and can be used in comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

autodoc reads DeclareOperations from comments

2 participants