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

Added guidelines for package authors from Example package #484

Merged
merged 2 commits into from
Nov 7, 2017

Conversation

olexandr-konovalov
Copy link
Member

@olexandr-konovalov olexandr-konovalov commented Jan 17, 2016

I have started to work on moving the guidelines for package authors from the Appendix of the Example package to the chapter on GAP packages in the reference manual.

This is work in progress - at the moment I copied and pasted the text from Appendix with minimal edits.

Please help to review the text and identify things that have to be rewritten, leaving your comments under corresponding lines in the commit.

UPDATE: this finally acquired some shape and I am happy to start the review process. Comments are welcome. You may decide to comment on the cumulative diff or on the two commits: the 1st commit just copies the text from the Appendix of the Example package manual, and the second edits it, so you can see which parts of the text are inherited from the old document, and which have been changed.

@olexandr-konovalov olexandr-konovalov added topic: documentation Issues and PRs related to documentation gapsagedays2016 Issues and PRs that arose at https://www.gapdays.de/gap-sage-days2016 labels Jan 17, 2016
@olexandr-konovalov
Copy link
Member Author

Seems like because of the diff of 2000+ lines one can't comment on individual lines of the commit :(

@olexandr-konovalov
Copy link
Member Author

This is a limitation of GitHub - it will not show diffs for commits larger than it seems 1500 lines.
I've now added the new material in two commits instead of one and force-pushed it. You will still not see the cumulative diff under "files changed" but you will be able to see the added text and leave your comments under "commits".

@olexandr-konovalov
Copy link
Member Author

This something that I still want and intend to do. Assigned to myself and set milestone 4.9.0.
Cross-referenced to gap-packages/example#8

@codecov
Copy link

codecov bot commented Mar 11, 2017

Codecov Report

Merging #484 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #484      +/-   ##
==========================================
+ Coverage   63.49%   63.49%   +<.01%     
==========================================
  Files         952      952              
  Lines      286804   286803       -1     
  Branches    12747    12747              
==========================================
+ Hits       182103   182104       +1     
+ Misses     101904   101897       -7     
- Partials     2797     2802       +5
Impacted Files Coverage Δ
src/hpc/thread.c 45.25% <0%> (-0.8%) ⬇️
src/stats.c 79.01% <0%> (-0.28%) ⬇️
src/lists.c 56.6% <0%> (-0.12%) ⬇️
src/hpc/threadapi.c 34.3% <0%> (ø) ⬆️
src/funcs.c 74.75% <0%> (+0.14%) ⬆️
src/objset.c 82.59% <0%> (+0.23%) ⬆️
src/iostream.c 46.64% <0%> (+0.39%) ⬆️
src/hpc/traverse.c 78.68% <0%> (+0.68%) ⬆️
lib/queue.g 69.59% <0%> (+3.19%) ⬆️

@olexandr-konovalov
Copy link
Member Author

I did some minor rewrites and added numerous TODOs to the package guidance - will try to get more progress this week, aiming at having this update for GAP 4.9 and making new release of the Example package with this appendix removed.

@olexandr-konovalov olexandr-konovalov added the gapdays2017-spring Issues and PRs that arose at https://www.gapdays.de/gapdays2017-spring label Mar 12, 2017
@olexandr-konovalov
Copy link
Member Author

Update: trying to make some progress with this during this week.

@olexandr-konovalov olexandr-konovalov force-pushed the package-manual branch 4 times, most recently from 8f109fa to f11a1b9 Compare September 17, 2017 17:33
@olexandr-konovalov olexandr-konovalov removed the do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) label Sep 17, 2017
@olexandr-konovalov
Copy link
Member Author

This finally acquired some shape and I am happy to start the review process. Comments are welcome. You may decide to comment on the cumulative diff or on the two commits: the 1st commit just copies the text from the Appendix of the Example package manual, and the second edits it, so you can see which parts of the text are inherited from the old document, and which have been changed.

@olexandr-konovalov olexandr-konovalov changed the title Added guidelines for package authors from Example package (WIP) Added guidelines for package authors from Example package Sep 17, 2017
or install new packages between upgrades of your &GAP; installation.
Before a package can be used it must be installed. A standard
installation of &GAP; already contains all currently redistributed with
&GAP; packages. Such combination of packages is checked for compatibility
Copy link
Member

Choose a reason for hiding this comment

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

Word order: "all GAP packages currently distributed with GAP".

Then: "This set of packages is checked ..."

the packages can be used immediately, but some of them may require further
installation steps such as compilation or installing additional software
to satisfy their dependencies. Most of the packages that require compilation
will be compiled in a single step by changing to the <F>pkg</F> directory
Copy link
Member

Choose a reason for hiding this comment

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

Will -> can

installation steps such as compilation or installing additional software
to satisfy their dependencies. Most of the packages that require compilation
will be compiled in a single step by changing to the <F>pkg</F> directory
of your &GAP; installation and calling <C>../bin/BuildPackages.sh</C> script.
Copy link
Member

Choose a reason for hiding this comment

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

"... calling the ... script."

of your &GAP; installation and calling <C>../bin/BuildPackages.sh</C> script.
<P/>
Also, since &GAP; packages are released independently of the main &GAP; system,
sometimes it may be sensible to upgrade or install new packages between
Copy link
Member

Choose a reason for hiding this comment

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

Not sure "sensible" is a good word choice here. Perhaps "useful"? And then perhaps clarify why/ when, e.g. like this: "... e.g. if a new version of a package adds new capabilities or bug fixes that you need."

<Ref Func="LoadPackage"/>.
Some &GAP; packages are loaded automatically with &GAP;. Those belong to
two categories: packages which are needed to start &GAP; (their list is
contained in <C>GAPInfo.Dependencies.NeededOtherPackages</C>), and packages
Copy link
Member

Choose a reason for hiding this comment

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

I'd make this explicit: "(as of this writing, the only such package is GAPDoc; the current list is contained in the ... variable)"

in loading it via <Ref Func="LoadPackage"/>, satisfying
all specified dependencies.
<P/>
You can also try to create a basic &GAP; package with the help of the
Copy link
Member

Choose a reason for hiding this comment

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

Remove "try" ?

And in light of the above, perhaps insert before this a paragraph which mentions validation and/or refs to ValidatePackageInfo, and something along the lines of "While this minimal PackageInfo.g is enough to let GAP load the package, it actually is missing many extra fields that every such file should contain, and which we require for a package to be redistributed with GAP. The command ValidatePackageInfo can be used to get a quick idea about which fields are missing:

gap> ValidatePackageInfo("PackageInfo.g");
#E  component `Subtitle' must be bound to a string
#E  component `Date' must be bound to a string of the form `dd/mm/yyyy'
#E  component `ArchiveURL' must be bound to a string started with http://, https:// or ftp://
#E  component `ArchiveFormats' must be bound to a string
#E  component `Status' must be bound to one of "accepted", "deposited", "dev", "other"
#E  component `README_URL' must be bound to a string started with http://, https:// or ftp://
#E  component `PackageInfoURL' must be bound to a string started with http://, https:// or ftp://
#E  component `AbstractHTML' must be bound to a string
#E  component `PackageWWWHome' must be bound to a string started with http://, https:// or ftp://
#E  component `ArchiveURLSubset' must be bound to a list of strings denoting relative paths to readable files or directories
#E  component `HTMLStart' must be bound to a string denoting a relative path to a readable file
#E  component `PDFFile' must be bound to a string denoting a relative path to a readable file
#E  component `SixFile' must be bound to a string denoting a relative path to a readable file
#E  component `LongTitle' must be bound to a string
false

At that point, it then is quite natural to bring up PackageMaker, and to mention that it takes care of almost all of that for you.

Copy link
Member

Choose a reason for hiding this comment

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

Aha, and a link to the "Creating the PackageInfo.g File" section should be added here, too. And the log I gave above could be added there (instead), I guess.

all specified dependencies.
<P/>
You can also try to create a basic &GAP; package with the help of the
tool called <Package>PackageMaker</Package>, created by Max Horn and
Copy link
Member

Choose a reason for hiding this comment

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

It is my hope that others will help out with that tool, so perhaps don't bring up my name with that? The manual doesn't mention Max N. and Frank for GAPDoc all the time either, I think.

<Log><![CDATA[
#############################################################################
##
#F ListDirectory([<dir>]) . . . . . . . . . . list the files in a directory
Copy link
Member

Choose a reason for hiding this comment

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

Personally, I would just ditch this line. It serves no purpose; but having it here makes people feel like they need to add it, increasing the work required to write comments. Also, these lines tend to get out of sync with the rest of the comment anyway."

This description was automatically be extracted to build the
manual source, if there is a <C>\Declaration</C> line in some
<C>.msk</C> file together with an appropriate configuration file.
<P/>
Copy link
Member

Choose a reason for hiding this comment

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

Personally, I'd remove the stuff between Historicallyand this point. Well, or perhaps instead move it to a new "historical" section (the information above is useful for understanding why some old code has comments like that). But other than that historical interest, I find it quite distracting. Prospective package authors have to slog through all of that for no good reason before they get to the stuff that actually matters for them.

<Ref Func="ValidatePackageInfo"/>.

</Section>

Copy link
Member

Choose a reason for hiding this comment

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

I stopped reading here for now. Perhaps somebody else can pick up reviewing after this, or perhaps I'll find some time and energy to do it later.

@olexandr-konovalov
Copy link
Member Author

@fingolfin this has been updated following your comments on the initial part of the chapter.

@olexandr-konovalov
Copy link
Member Author

I have taken all feedback received so far from @fingolfin into account. I would like to see this merged for 4.9 beta, and then update the Example package, to remove the old version of this text from there. We then may point package author to its new location in the Ref manual, and then hopefully get further feedback from them after publishing beta.

and uses the value assigned to <C>GAParch</C> in the file <F>sysinfo.gap</F>,
created when &GAP; was compiled to determine the
compilation architecture, inserts this in place of the string <C>@GAPARCH@</C>
in <F>Makefile.in</F> and creates a file <F>Makefile</F>. When <C>make</C> is
Copy link
Member

Choose a reason for hiding this comment

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

This information is outdated with the new build sys. While this still works (due to the "compatibility mode"), the plan is for sysinfo.gap to go away, or at last change substantially. I am not comfortable telling people to use it this way. :/

That said, right now the alternatives are not yet ready, so we may have no choice. I'll try to work on the package build system side next week, but unless I make progress, we'll just have to merge this, but I must make a big note somewhere to remind me that this piece of text exists and needs to be updated.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've put the comment in the XML source about this.

GAP4stones: 3333
true
]]></Log>
Tests which produce extended output and/or requires substantial runtime
Copy link
Member

Choose a reason for hiding this comment

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

requires -> require

works. Such tests should be included in the package distribution, and the
responsibility for ensuring that they pass stays with package authors.
<P/>
Second, the package should cleanly intergate into the &GAP; system and other
Copy link
Member

Choose a reason for hiding this comment

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

intergate -> integrate

documentation format that is defined in and can be used with the &GAPDoc;
package (see&nbsp;<Ref Chap="Introduction and Example" BookName="gapdoc"/>).
<P/>
There should be at least a text version of your documenation provided for use
Copy link
Member

Choose a reason for hiding this comment

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

documenation -> documentation

<P/>
Additionally, there is a recommended naming convention that the &GAP; core
system and &GAP; packages should not use global variables starting in the
lowercase. This allows to reserve variables with names starting in lowecase
Copy link
Member

Choose a reason for hiding this comment

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

lowecase -> lowercase

the file <F>doc/example.xml</F> of the <Package>Example</Package> package.
(it uses <Package>GAPDoc</Package> for documentation -
you will need some different scheme to achieve this using
the <F>gapmacro.tex</F> format).
Copy link
Member

Choose a reason for hiding this comment

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

Remove the last part in parenthesis?

Copy link
Member Author

Choose a reason for hiding this comment

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

Checked and removed some other mentions of gapmacro. Now the only one left is "Some packages still use for their manuals the old gapmacro format, support for which may be discontinued in the future. We encourage authors of those packages to eventually convert their documentation &GAPDoc;."

</Subsection>


<Subsection Label
Copy link
Member

Choose a reason for hiding this comment

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

aplhabetical -> alphabetical

</Subsection>


<Subsection Label
Copy link
Member

Choose a reason for hiding this comment

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

permisisons -> permissions

</Subsection>


<Subsection Label
Copy link
Member

Choose a reason for hiding this comment

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

maybe replace that with .gitignore?

@olexandr-konovalov
Copy link
Member Author

@fingolfin thanks. Addressed comments and run a spellcheck. Should be ready to merge if Travis tests pass.

(<URL>https://www.gap-system.org</URL>)
and in the &GAP; package <Package>Example</Package>
(see <URL>https://www.gap-system.org/Packages/example.html</URL>).
There are possibilities to get a package distributed together with
&GAP; and it is possible to submit a package to a formal refereeing
process.
<P/>
In this chapter we describe how to use existing packages.
In this chapter first we describe how to use existing packages,
Copy link
Contributor

Choose a reason for hiding this comment

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

we first

@@ -27,26 +17,36 @@ integrated into the &GAP; help system.
All &GAP; users who develop new code are invited to share
the results of their efforts with other &GAP; users by making
the code and its documentation available in form of a package.
Information how to do this is available from the &GAP; Web pages
Information how to do this is available from the &GAP; website
(<URL>https://www.gap-system.org</URL>)
and in the &GAP; package <Package>Example</Package>
(see <URL>https://www.gap-system.org/Packages/example.html</URL>).
There are possibilities to get a package distributed together with
Copy link
Contributor

Choose a reason for hiding this comment

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

This sentence needs changing. Perhaps (working from the website): Once a package works and has documentation of its functionality, you should consider how to make it available to other GAP users as a deposited package. Please contact support@gap-system.org so that the GAP Group can make the necessary checks. In these are satisfied, the package can be included in the distribution update mechanism.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd leave it as it is - there are more details below about depositing a package.

that is they will be loaded automatically with &GAP;,
others must in each case be separately loaded by a call to
<Ref Func="LoadPackage"/>.
Some &GAP; packages are loaded automatically with &GAP;. Those belong to
Copy link
Contributor

Choose a reason for hiding this comment

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

Surely the average (or casual) GAP user just wants to know how to load a package? So include here the material in the LoadPackage subsection which refers to this, and move the automatic loading stuff to a later subsection.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've reordered the text to mention LoadPackage at the start of the Section.

<Ref Sect="sect:gap.ini"/>) or start &GAP; with <C>-O</C> command line
option (this may also cause problems with loading other packages using
<Q>obsolete</Q> variables).

Choose a reason for hiding this comment

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

gap -O might be a bit discouraging; probably gap -A -O would be better, since even many default autoloaded packages contain obsolete functions

Copy link
Member Author

Choose a reason for hiding this comment

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

@pedritomelenas thanks, good suggestion!

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.

It doesn't look like many people are interested in further checking this. Yet it represents a clear improvement over what is currently there. So, I am fine with merging it now and improving it later, in-tre.

@fingolfin fingolfin merged commit c189950 into gap-system:master Nov 7, 2017
@olexandr-konovalov olexandr-konovalov deleted the package-manual branch November 14, 2017 22:41
@olexandr-konovalov olexandr-konovalov added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Dec 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gapdays2017-spring Issues and PRs that arose at https://www.gapdays.de/gapdays2017-spring gapsagedays2016 Issues and PRs that arose at https://www.gapdays.de/gap-sage-days2016 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.

4 participants