View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0027028Open CASCADEOCCT:Documentationpublic2015-12-22 15:112021-10-29 11:19
ReporterabvAssigned To 
PrioritynormalSeverityfeature 
Status newResolutionopen 
Target VersionUnscheduled 
Summary0027028: Documentation - define common style for commit messages
DescriptionIn context of revision of OCCT Contribution Workflow document (see 0024514 and 0026425) we have tried to elaborate general guidelines for style to be used for the commit messages. Several formulations have been proposed, but no common understanding has been reached. Hence, it is separated to this issue.

Some variants that have been discussed are:

- Use plain English
- Use simple tenses
- Use primary tenses
- Use present simple, past simple and present perfect tenses
- Use impersonal sentences
- Use third-person narration
- Be logical and put things simply

In addition, several pages of annotated examples of commit descriptions have been extracted in the last version of CR24514_2, this part of the document is recorded here in "Additional details" field.
Additional information
and documentation updates		@section occt_contribution_app2 Appendix 2: Examples of commit messages

#### Simple description of the fix

> *The exception in method BRepFill_Cone::Truncate caused by hard-coded number of symbols in the parameter for truncation operation has been fixed. Now the parameter provided by method BRepFill_Cone::Parameter is stored in a variable, which is used for truncation.*

Here, in this example:
 * the **Problem** is "hard-coded number of symbols in the parameter for truncation operation"
 * the **Change** is "The exception in method BRepFill_Cone::Truncate has been fixed"
 * the **Result** is "Now the parameter provided by method BRepFill_Cone::Parameter is stored in a variable, which is used for truncation".
 

Other cases may require an even more detailed description, or vice versa, some element can be not worth indicating.

#### Exposing the Problem

The **Problem** can be omitted if it is stated correctly and in full in the **Summary** of the issue.

For example:

> *Summary: Error in IntPatch_PrmPrmIntersection: change of local resolution leads to break of walking line.*

completely exposes the **Problem** and the description

> *The method IntWalk_Pwalking::TestDeflection has been modified to take into account the local resolution of the chosen surface during the computation of the local step and the maximum step.*

consists only of:
 * the **Change**, which is "The method IntWalk_Pwalking::TestDeflection has been modified", and
 * the **Result** "to take into account the local resolution of the chosen surface", etc.

However, the Summary may need additional clarification, for example:

> *Summary: Offset on faces with opposite orientation.*
>
> *The offset algorithm has been improved in method BRepOffset_Offset::Init to properly calculate normals if mirror transformations are associated with input objects.*

Here, "if mirror transformations are associated with input objects" describes the **Problem**.

Structurally, the **Problem** can be introduced, for example:

* By a separate sentence, probably starting with "previously", "initially", "in earlier OCCT versions", etc. For example:
> *Previously the method XCAFDoc_ShapeTool::UpdateAssembly() rebuilt the shape of assembly, however, it did not follow the back-references, i.e. the users of the assembly. Now this method checks back-references in the bottom-up direction to ensure the shape data consistency in an XCAF document.*

* By an "if" or "when" clause defining the prerequisites of the erroneous behavior:
> *The function ComputePurgedWLine() now can delete excess points in the walking line if the distance between points is too small or they lie in one pipe without big jump on chord length.*

* By a complement to such verbs as "avoid" or "eliminate" :
> *Equality checks have been added to camera modification methods in Graphic3d_Camera class to avoid camera updates when performing identity operations.*

* By a phrase "the problem/issue with"
> *The problem with a failure to resize objects when the view area is maximized has been fixed in Qt samples.*

It can be noted that stating only the **Problem** and the **Change** can be sufficient in case of regressions, when the behavior of algorithms is restored rather than modified.

#### Exposing the Change

The **Change** is basically a mention of the introduced or amended function/method/class.

It can be introduced by a sentence with such verbs as improve, modify, fix, add, change, enhance, etc. in present perfect tense.

If you wish to emphasize the **Result** rather than the **Change**, the latter can be reduced to a modifier of place.

I.e. the message
> *Function() has been corrected to avoid compiler warnings*

can be paraphrased as

> *Compiler warnings are now avoided in Function().*

The **Change** can be omitted in the message if only one method has been amended, and that can be seen in GIT.

* In complex fixes or improvements when there are several methods/classes concerned by the fix, and correspondingly several different statements, it is necessary to indicate which corresponds to which:
> *Protection against 0 dimensions has been added in OpenGl_FrameBuffer::Init().*
> *New method V3d_View::IsInvalidated() checks view cache validation state.*
> *ViewerTest::ViewerInit() now creates a virtual window without decorations on Windows.*

* If numerous similar fixes are applied in one (or several) classes, packages or toolkits, it is possible to generalize:
> *BRepBuilderAPI package has been revised to avoid declaring methods operator TopoDS_Shape and Shape as const, which can cause a compilation error.*

* Same for more global revisions:
> *OCCT code has been refactored to remove redundant const qualifiers of return types of functions returning values.*

Presenting only the **Change** is enough when we speak about removal of obsolete entities, however it is advisable to mention which entities can be used instead.

> *Package SortTools and its derived classes have been removed and replaced throughout OCCT by STL sorting algorithms (i.e. std::sort).*

Or:

> *Class BOPCol_Array1 has been removed.*
> *Instead, new method Ncollection_BaseVector::SetIncrement allows setting the size of increment dynamically.*
> *Classes from package BOPDS have been modified accordingly.*

#### Exposing the Result

The result describes the actual state or behavior of the platform and thus is usually introduced in the main clause in present simple tense.

Usually it conveys the most interesting information for the end-user, so this is really preferable to concentrate on this part of the message.

I.e. instead of focusing on the **Problem**:
> *The use of reference to a destroyed temporary object has been fixed in method Adaptor3d_SurfaceOfRevolution::GetType().*

it is better to focus on the **Result**:
> *The method Adaptor3d_SurfaceOfRevolution::GetType() now makes a copy of temporary object for further use.*

Here are some typical examples of results and appropriate commit messages:

* Implementation of a new method/class:
> *New method CheckFace::Point allows checking if the point is IN the face.*

* Improvement of an existing algorithm:
> *The concatenation algorithm in class BSpline_Surface now works with periodic BSpline surfaces.*

* Introduction of rules and limitations:
> *Comparison of handle to NULL has become impossible. Method IsNull() should be used instead.*

#### Description of a large-scale development

The implementation of new functionalities or functional overhauls can contain numerous modifications concerning numerous packages or classes in one package.
The description of such implementation should include two elements:
* General description or Summary, which usually defines the **Problem**;
* List of modifications with necessary comments, which include the **Changes** and the **Results**.

According to the requirements of section @ref occt_contribution_workflow_issue, the purpose of an improvement should be already given in the Issue Description.

However, if the Issue Description is missing or if it describes the problems rather then their solution or if some other important information is missing, it should be given in the commit message.

Here is the example of information about numerous changes in connection with the issue that should be provided:

> *B-spline cache has been separated into classes BSplCLib_Cache for 2D and 3D curves and BSplSLib_Cache for surfaces. They are now used in the corresponding adaptor classes Geom2dAdaptor_Curve, GeomAdaptor_Curve and GeomAdaptor_Surface when the curve or surface is a B-spline.*
>
> *Other related changes are:*
> - *Classes BOPAlgo_WireSplitter, BOPTools_AlgoTools, BRepLib_CheckCurveOnSurface and ShapeAnalysis_Wire now use adaptors for B-spline calculations instead of geometric entities (curves or surfaces);*
> - *Classes from Geom2dAdaptor and GeomAdaptor packages now use the adaptor for the base curve for evaluation of offset curves*;
> - *The derivatives of a surface of revolution now can be precisely calculated in class Geom_SurfaceOfRevolution for the points of surface placed on the axis of revolution.*

Some recommendations:
* Try to generalize similar modifications in one entry
* If the modification is meaningless or too small, do not mention it.
* Sometimes more modifications are added while the fix is reviewed via several iterations; early modifications can be overridden or dropped. Make sure to update the description of the commit when you send the fix for review.
 
TagsNo tags attached.
Test case number

Activities

ysn

2015-12-23 15:37

developer   ~0049516

A note about Simple English approaches - basically none of them helps us.
 
1) https://en.wikipedia.org/wiki/Plain_English
https://en.wikipedia.org/wiki/List_of_plain_English_words_and_phrases
 
This is an effort to reduce usage of borrowings from French and Latin. It is not our problem at all.
 
2) https://en.wikipedia.org/wiki/Simplified_Technical_English aims to avoid ambiguity in crucial technical descriptions.
It has its own vocabulary– mainly characterized by very narrow usage of all terms. I don’t think that we can require that people invest efforts into its learning.
 
3) I can admit that Simple English has its merits https://simple.wikipedia.org/wiki/Wikipedia:How_to_write_Simple_English_pages.
Basically it aims to improve readability by replacing uncommon terms with synonyms. Again, it is not the problem of our developers. Their problem is not too rich vocabulary, their problem is poor vocabulary. If they know a term, they should better use it, instead of inventing.

Issue History

Date Modified Username Field Change
2015-12-22 15:11 abv New Issue
2015-12-22 15:11 abv Assigned To => ysn
2015-12-22 15:11 abv Summary Doxumentation -- define common style for commit messages => Documentation -- define common style for commit messages
2015-12-23 15:37 ysn Note Added: 0049516
2021-10-29 11:19 kgv Assigned To ysn =>
2021-10-29 11:19 kgv Severity minor => feature
2021-10-29 11:19 kgv Summary Documentation -- define common style for commit messages => Documentation - define common style for commit messages