Skip to content
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

SuggestedSpellings extended and renamed to HELP_SEARCH_ALTERNATIVES #1144

Merged
merged 1 commit into from
Feb 21, 2017

Conversation

olexandr-konovalov
Copy link
Member

@olexandr-konovalov olexandr-konovalov commented Feb 15, 2017

This addresses #1019.

Please make sure that this pull request:

  • is submitted to the correct branch (the stable branch is only for bugfixes)
  • contains an accurate description of changes for the release notes below
  • provides new tests or relies on existing ones
  • correctly refers to other issues and related pull requests

Tick all what applies to this pull request

  • Adds new features
  • Improves and extends functionality
  • Fixes bugs that could lead to crashes
  • Fixes bugs that could lead to incorrect results
  • Fixes bugs that could lead to break loops

Write below the description of changes (for the release notes)

The search for the documentation of system setters and testers
such as e.g. ?SetIsMapping and ?HasIsMapping will return
corresponding attributes and properties. e.g. IsMapping.

@codecov
Copy link

codecov bot commented Feb 15, 2017

Codecov Report

Merging #1144 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1144      +/-   ##
==========================================
+ Coverage   59.21%   59.25%   +0.04%     
==========================================
  Files         434      434              
  Lines      230814   230825      +11     
==========================================
+ Hits       136682   136786     +104     
+ Misses      94132    94039      -93
Impacted Files Coverage Δ
lib/helpbase.gi 41.74% <100%> (+12.42%)
lib/grppc.gi 61.39% <ø> (-0.17%)
lib/tietze.gi 41.99% <ø> (+0.13%)
lib/grpfp.gi 61.29% <ø> (+0.19%)
lib/stbcrand.gi 90.46% <ø> (+0.29%)
lib/matint.gi 68.34% <ø> (+0.37%)
lib/wordass.gi 39.65% <ø> (+0.52%)
lib/helpdef.gi 38.16% <ø> (+0.66%)
lib/liefam.gi 40.79% <ø> (+0.79%)
lib/kernel.g 61.9% <ø> (+6.34%)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 436cdb0...06ffefa. Read the comment docs.

@olexandr-konovalov
Copy link
Member Author

olexandr-konovalov commented Feb 15, 2017

For any search term starting with Has or Set, this will search for HasSomething, SetSomething and Something. This is a slight overhead which I believe may be neglected. Of course one could decide to search only for Something plus the original search term (just in case it does not mean setter/tester, like e.g. in SetupCache or HastaLaVista - in this case it will also search for HasupCache and upCache etc. - this is also neglectible).

@olexandr-konovalov olexandr-konovalov added the topic: documentation Issues and PRs related to documentation label Feb 15, 2017
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.9.0 milestone Feb 16, 2017
@olexandr-konovalov olexandr-konovalov force-pushed the help-setter-tester branch 2 times, most recently from 43aa919 to 06ffefa Compare February 16, 2017 12:24
@olexandr-konovalov
Copy link
Member Author

This should be ready to merge now. I've updated this PR to achieve target goals for test coverage. That revealed that one of the tests, (formerly from helpsys.tst, now helptools.tst) became broken because of set stabiliser. I've added a check that the next character after set or has is not a space.

Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Looks good to me (modulo typos) and worked in a brief test.

lib/helpbase.gi Outdated

# Now chop the string 'topic' into a list of lists, each of them either
# a list of all variants from the respective spelling pattern or just
# a one-element list with the "glueing" string between two pattersn or
Copy link
Member

Choose a reason for hiding this comment

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

typo: pattersn -> patterns

@@ -0,0 +1,17 @@
# This test checks various auxiliary functions used by the help system.
#
# For the test that systematically checks each mahual section, see
Copy link
Member

Choose a reason for hiding this comment

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

typo: mahual -> manual

gap> HELP_SEARCH_ALTERNATIVES("hasismapping");
[ "hasismapping", "ismapping", "setismapping" ]
gap> HELP_SEARCH_ALTERNATIVES("setismapping");
[ "hasismapping", "ismapping", "setismapping" ]
Copy link
Member

Choose a reason for hiding this comment

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

perhaps also include HELP_SEARCH_ALTERNATIVES("ismapping") ?

Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Actually, I just found one caveat: Try ?hash. Without this PR, it lists four (relevant) help entries for me). Without it, it turns this into a search for "h" and thus finds countless entries.

Similar for sets, SetX, ...

So at the very least, I guess it should require the search string to have more than 4-5 letters.

Better would be if it only took the alternates into account if there are no its. I.e. if "topic" has a hit, just use it, if there are no hits, remove the has resp. set prefix. Of course such a change is more complicated than what is in this PR :-/

@codecov-io
Copy link

codecov-io commented Feb 18, 2017

Codecov Report

Merging #1144 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1144      +/-   ##
==========================================
+ Coverage   59.39%   59.43%   +0.03%     
==========================================
  Files         434      434              
  Lines      230498   230509      +11     
==========================================
+ Hits       136913   137004      +91     
+ Misses      93585    93505      -80
Impacted Files Coverage Δ
lib/helpbase.gi 41.74% <100%> (+12.42%)
src/system.c 62.84% <ø> (-1.05%)
lib/tietze.gi 41.99% <ø> (-0.6%)
lib/straight.gi 25.28% <ø> (-0.42%)
lib/stbcrand.gi 90.46% <ø> (+0.09%)
lib/grpfp.gi 61.29% <ø> (+0.19%)
lib/helpdef.gi 38.16% <ø> (+0.66%)
lib/grppcint.gi 99.32% <ø> (+0.67%)
lib/polyrat.gi 59.46% <ø> (+0.75%)
lib/csetperm.gi 91.35% <ø> (+3.7%)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d7f2b93...a58f136. Read the comment docs.

@olexandr-konovalov
Copy link
Member Author

@fingolfin I've made an update. Good catch about hash and SetX, thanks!

lib/helpbase.gi Outdated

for topic in topics do
if Length(topic) > 4 and topic{[1..3]} in [ "has" , "set" ] and topic[4]<>' ' then
shorttopic := topic{[4..Length(topic)]};
Copy link
Member

Choose a reason for hiding this comment

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

I think there should be some comments here. One that explains what's going on ("ensure HasIsMapping is found in help even if only IsMapping ismdocumentd"), another which explains that x > 4 is not an off-by-one accident, but rather intentionally (and also what the intention is). Otherwise I am afraid somebody might "fix" it someday .

Now it also ensures that the search for system setters and testers
such as e.g. ?SetIsMapping and ?HasIsMapping will return corresponding
attributes and properties. e.g. IsMapping.
@olexandr-konovalov
Copy link
Member Author

@fingolfin comments added, good suggestion.

@fingolfin fingolfin merged commit 86321d9 into gap-system:master Feb 21, 2017
@olexandr-konovalov olexandr-konovalov added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Jan 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: added PRs introducing changes that have since been mentioned in the release notes topic: documentation Issues and PRs related to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants