Skip to content

Commit

Permalink
Replace all Module's with ModuleScope (#461)
Browse files Browse the repository at this point in the history
## Changes
* Closes #460.
* Replaces (almost) all `Module`'s with `ModuleScope`'s.
* Adds ```PackageImport["GeneralUtilities`"]``` where `ModuleScope` was used.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/maxitg/setreplace/461)
<!-- Reviewable:end -->
  • Loading branch information
daneelsan authored Oct 19, 2020
1 parent 7b9edf8 commit e78a32c
Show file tree
Hide file tree
Showing 27 changed files with 237 additions and 168 deletions.
60 changes: 57 additions & 3 deletions .github/CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,8 @@ One way to implement such argument checking is to make special `valid*Q` functio
A better approach is to setup the function to catch exceptions, i.e.,

```wl
MakeUniverse[args___] := Module[{result = Catch[makeUniverse[args]]},
MakeUniverse[args___] := ModuleScope[
result = Catch[makeUniverse[args]];
result /; result =!= $Failed
]
```
Expand Down Expand Up @@ -364,18 +365,71 @@ In addition to that, here are some more-or-less established rules:
]
```

```wl
If[MatchQ[hypergraph, {__List}],
Sort @ Union @ Catenate @ hypergraph,
Throw @ $Failed
]
```

* However, close the brackets of ordinary functions on the same line as the last argument:

```wl
veryLongFunctionCall[
longArgument1, longArgument2, longArgument3]
```

* The function arguments should either all go on the same line, or should each be put on a separate line (except for special cases where a large quantity of short arguments is used).
* The function arguments should either all go on the same line, or should each be put on a separate line (except for special cases where a large quantity of short arguments is used):

```wl
wolframModelPlot[
edges_,
edgeType_,
styles_,
hyperedgeRendering_,
vertexCoordinates_,
vertexLabels_,
vertexSize_,
arrowheadLength_,
maxImageSize_,
background_,
graphicsOptions_] := Catch[
...]
```

* Avoid using [`Flatten`](https://reference.wolfram.com/language/ref/Flatten.html) and [`ReplaceAll`](https://reference.wolfram.com/language/ref/ReplaceAll.html) without explicit level arguments. That is because it is very easy to accidentally assume that the user's input is not a [`List`](https://reference.wolfram.com/language/ref/List.html) (e.g., a vertex name), even though it can be, in which case you would [`Flatten`](https://reference.wolfram.com/language/ref/Flatten.html) too much, and cause a weed. It is preferred to use [`Catenate`](https://reference.wolfram.com/language/ref/Catenate.html) and [`Replace`](https://reference.wolfram.com/language/ref/Replace.html) instead of these functions.
* Similar issue could happen with [`Thread`](https://reference.wolfram.com/language/ref/Thread.html), especially when used to thread a single element over multiple. For example, it is easy to assume naively that `Thread[x -> {1, 2, 3}]` would always yield `{x -> 1, x -> 2, x -> 3}`. Except, sometimes it might be called as `With[{x = {4, 5, 6}}, Thread[x -> {1, 2, 3}]]`.
* Use uppercase camel for public symbols, lowercase camel for internal (including PackageScope) symbols.
* Use uppercase camel for public symbols, lowercase camel for internal (including PackageScope) symbols:

```wl
PackageExport["WolframModelEvolutionObject"]
PackageScope["propertyEvaluate"]
```

* Start global constants with `$`, whether internal or public, and tags (such as used in [`Throw`](https://reference.wolfram.com/language/ref/Throw.html) or [`Sow`](https://reference.wolfram.com/language/ref/Sow.html), or as generic enum labels) with `$$`. Global pure functions (defined as [`OwnValues`](https://reference.wolfram.com/language/ref/OwnValues.html)) should still be treated as ordinary (e.g., [`DownValues`](https://reference.wolfram.com/language/ref/DownValues.html)) functions and not start with `$`, unless they are public, in which case they should start with `$` and end with the word `Function`.
* Use the macros `ModuleScope` and `Scope` (defined in ``"GeneralUtilities`"``) instead of `Module` and `Block` (respectively) when defining functions of the form `f[x__] := (Module|Block)[...]`. The main benefit of using them is that there is no need to specify a list of local variables (i.e. `{localVar1, localVar2, ...}`) at the begining, as a list of local variables will be automatically generated by looking for expressions of the form `Set` (`=`) or `SetDelayed` (`:=`) anywhere in the body of the function (See `?Scope` and [#460](https://github.com/maxitg/SetReplace/pull/460) for more information). For example:

```wl
example[hypergraph1_, hypergraph2_] := ModuleScope[
{vertexList1, vertexList2} = getVertexList /@ {hypergraph1, hypergraph2};
If[Length @ hypergraph1 >= Length @ hypergraph2,
size = Length @ hypergraph1,
size = Length @ hypergraph2
];
]
```
is expanded to:

```wl
example[hypergraph1_, hypergraph2_] := Module[{vertexList1, vertexList2, size},
{vertexList1, vertexList2} = getVertexList /@ {hypergraph1, hypergraph2};
If[Length @ hypergraph1 >= Length @ hypergraph2,
size = Length @ hypergraph1,
size = Length @ hypergraph2
];
]
```

#### C++
The code should follow [Google C++ Style](https://google.github.io/styleguide/cppguide.html) guidelines, save for the
Expand Down
8 changes: 5 additions & 3 deletions Kernel/GeneralizedGridGraph.m
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
Package["SetReplace`"]

PackageImport["GeneralUtilities`"]

PackageExport["GeneralizedGridGraph"]

(* Documentation *)
Expand Down Expand Up @@ -31,7 +33,8 @@

(* Implementation *)

GeneralizedGridGraph[args___] := Module[{result = Catch[generalizedGridGraph[args]]},
GeneralizedGridGraph[args___] := ModuleScope[
result = Catch[generalizedGridGraph[args]];
result /; result =!= $Failed
]

Expand Down Expand Up @@ -69,8 +72,7 @@
Throw[$Failed];
)

generalizedGridGraphExplicit[dimSpecs_, opts___] := Module[{
edgeStyle, vertexNamingFunction, edges, directionalEdgeStyle},
generalizedGridGraphExplicit[dimSpecs_, opts___] := ModuleScope[
{edgeStyle, vertexNamingFunction} = OptionValue[GeneralizedGridGraph, {opts}, {EdgeStyle, "VertexNamingFunction"}];
edges = singleDimensionEdges[dimSpecs, #] & /@ Range[Length[dimSpecs]];
directionalEdgeStyle = EdgeStyle -> If[
Expand Down
12 changes: 7 additions & 5 deletions Kernel/HypergraphAutomorphismGroup.m
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
Package["SetReplace`"]

PackageImport["GeneralUtilities`"]

PackageExport["HypergraphAutomorphismGroup"]

(* Documentation *)
Expand All @@ -14,7 +16,8 @@

(* Implementation *)

HypergraphAutomorphismGroup[args___] := Module[{result = Catch[hypergraphAutomorphismGroup[args]]},
HypergraphAutomorphismGroup[args___] := ModuleScope[
result = Catch[hypergraphAutomorphismGroup[args]];
result /; result =!= $Failed
]

Expand All @@ -28,8 +31,8 @@
removeAuxiliaryElements[GraphAutomorphismGroup[binaryGraph], binaryGraph, e]
]

toStructurePreservingBinaryEdges[hyperedge_] := Module[{
edgeVertices = Table[edge[Unique[v, {Temporary}]], Length[hyperedge]]},
toStructurePreservingBinaryEdges[hyperedge_] := ModuleScope[
edgeVertices = Table[edge[Unique[v, {Temporary}]], Length[hyperedge]];
Join[
EdgeList[PathGraph[edgeVertices, DirectedEdges -> True]],
Thread[DirectedEdge[edgeVertices, hyperedge]]]
Expand All @@ -42,8 +45,7 @@
and deleted), generators affecting both (which will be trimmed), and generators of original vertices only
(which will be preserved).*)

removeAuxiliaryElements[group_, graph_, hypergraph_] := Module[{
trueVertexIndices, binaryGraphIndexToVertex, vertexToHypergraphIndex},
removeAuxiliaryElements[group_, graph_, hypergraph_] := ModuleScope[
trueVertexIndices = Position[VertexList[graph], Except[_edge], {1}, Heads -> False][[All, 1]];
binaryGraphIndexToVertex = Thread[trueVertexIndices -> VertexList[graph][[trueVertexIndices]]];
vertexToHypergraphIndex = Thread[vertexList[hypergraph] -> Range[Length[binaryGraphIndexToVertex]]];
Expand Down
5 changes: 4 additions & 1 deletion Kernel/HypergraphUnifications.m
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
Package["SetReplace`"]

PackageImport["GeneralUtilities`"]

PackageExport["HypergraphUnifications"]

(* Documentation *)
Expand All @@ -16,7 +18,8 @@

(* Implementation *)

HypergraphUnifications[args___] := Module[{result = Catch[hypergraphUnifications[args]]},
HypergraphUnifications[args___] := ModuleScope[
result = Catch[hypergraphUnifications[args]];
result /; result =!= $Failed
]

Expand Down
10 changes: 6 additions & 4 deletions Kernel/HypergraphUnificationsPlot.m
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
Package["SetReplace`"]

PackageImport["GeneralUtilities`"]

PackageExport["HypergraphUnificationsPlot"]

(* Documentation *)
Expand All @@ -14,7 +16,8 @@

(* Implementation *)

HypergraphUnificationsPlot[args___] := Module[{result = Catch[hypergraphUnificationsPlot[args]]},
HypergraphUnificationsPlot[args___] := ModuleScope[
result = Catch[hypergraphUnificationsPlot[args]];
result /; result =!= $Failed
]

Expand All @@ -26,8 +29,7 @@

HypergraphUnificationsPlot::emptyEdge = "Empty edges are not supported.";

hypergraphUnificationsPlot[e1_, e2_, opts : OptionsPattern[]] := Module[{
unifications, automaticVertexLabelsList, vertexLabels, edgeStyle},
hypergraphUnificationsPlot[e1_, e2_, opts : OptionsPattern[]] := ModuleScope[
If[Length[Cases[Join[e1, e2], {}]] > 0,
Message[HypergraphUnificationsPlot::emptyEdge];
Throw[$Failed];
Expand All @@ -49,7 +51,7 @@
{unifications[[All, 1]], unifications[[All, 2]], unifications[[All, 3]], automaticVertexLabelsList}]
]

unificationVertexLabels[e1_, e2_][unification_, edgeMapping1_, edgeMapping2_] := Module[{labels1, labels2},
unificationVertexLabels[e1_, e2_][unification_, edgeMapping1_, edgeMapping2_] := ModuleScope[
{labels1, labels2} =
Merge[Reverse /@ Union[Catenate[Thread /@ Thread[#1[[Keys[#2]]] -> unification[[Values[#2]]]]]], Identity] & @@@
{{e1, edgeMapping1}, {e2, edgeMapping2}};
Expand Down
26 changes: 13 additions & 13 deletions Kernel/RulePlot.m
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
Package["SetReplace`"]

PackageImport["GeneralUtilities`"]

(* Documentation *)

$newOptions = {
Expand Down Expand Up @@ -53,7 +55,8 @@
(* Evaluation *)

WolframModel /: func : RulePlot[wm : WolframModel[args___] /; Quiet[Developer`CheckArgumentCount[wm, 1, 1]], opts___] :=
Module[{result = rulePlot$parse[{args}, {opts}]},
ModuleScope[
result = rulePlot$parse[{args}, {opts}];
If[Head[result] === rulePlot$parse,
result = $Failed];
result /; result =!= $Failed
Expand All @@ -62,7 +65,7 @@
(* Evolution object *)

WolframModelEvolutionObject /: RulePlot[evo : WolframModelEvolutionObject[data_ ? evolutionDataQ], opts___] :=
Module[{result},
ModuleScope[
result = rulePlot$parse[{evo["Rules"]}, {opts}];
If[Head[result] === rulePlot$parse,
result = $Failed];
Expand Down Expand Up @@ -122,7 +125,7 @@
False
]

correctSpacingsQ[opts_] := Module[{spacings, correctQ},
correctSpacingsQ[opts_] := ModuleScope[
spacings = OptionValue[RulePlot, opts, Spacings];
correctQ = MatchQ[spacings, Automatic | (_ ? NumericQ) | {Repeated[{Repeated[_ ? NumericQ, {2}]}, {2}]}];
If[!correctQ, Message[RulePlot::invalidSpacings, spacings]];
Expand Down Expand Up @@ -215,7 +218,7 @@
vertexSize_,
arrowheadLength_,
background_,
graphicsOpts_] := Module[{explicitSpacings, explicitAspectRatio, singlePlots, shapes, plotRange},
graphicsOpts_] := ModuleScope[
explicitSpacings = toListSpacings[Replace[spacings, Automatic -> style[$lightTheme][$ruleSidesSpacing]]];
hypergraphPlots =
rulePartsPlots[
Expand Down Expand Up @@ -243,8 +246,8 @@

aspectRatio[{{xMin_, xMax_}, {yMin_, yMax_}}] := (yMax - yMin) / (xMax - xMin)

aspectRatioFromPlotRanges[plotRanges_] := Module[{
singleAspectRatios = aspectRatio /@ plotRanges, minMax},
aspectRatioFromPlotRanges[plotRanges_] := ModuleScope[
singleAspectRatios = aspectRatio /@ plotRanges;
minMax = MinMax[singleAspectRatios];
Switch[minMax,
_ ? (Max[#] < 1 &), Max[minMax, style[$lightTheme][$rulePartsAspectRatioMin]],
Expand All @@ -266,8 +269,7 @@
edgePolygonStyle_,
vertexSize_,
arrowheadLength_][
rule_] := Module[{
vertexCoordinateRules, ruleSidePlots, plotRange},
rule_] := ModuleScope[
vertexCoordinateRules = Join[
ruleCoordinateRules[edgeType, hyperedgeRendering, externalVertexCoordinateRules, rule],
externalVertexCoordinateRules];
Expand All @@ -294,7 +296,7 @@

connectedQ[edges_] := ConnectedGraphQ[Graph[UndirectedEdge @@@ Catenate[Partition[#, 2, 1] & /@ edges]]]

layoutReferenceSide[in_, out_] := Module[{inConnectedQ, outConnectedQ},
layoutReferenceSide[in_, out_] := ModuleScope[
{inConnectedQ, outConnectedQ} = connectedQ /@ {in, out};
If[inConnectedQ && !outConnectedQ, Return[out]];
If[outConnectedQ && !inConnectedQ, Return[in]];
Expand All @@ -308,8 +310,7 @@
sharedRuleElements[in_ -> out_] := multisetIntersection @@ (Join[vertexList[#], #] & /@ {in, out})

(* returns {shapes, plotRange} *)
combinedRuleParts[sides_, plotRange_, spacings_, rulePartsAspectRatio_] := Module[{
xScaleFactor, yScaleFactor, maxRange, xRange, yRange, xDisplacement, frame, separator},
combinedRuleParts[sides_, plotRange_, spacings_, rulePartsAspectRatio_] := ModuleScope[
xScaleFactor = Min[1, 1 / rulePartsAspectRatio];
yScaleFactor = Min[1, rulePartsAspectRatio];
maxRange = Max[
Expand Down Expand Up @@ -351,8 +352,7 @@
separatorPlotRange_,
relativeSeparatorWidth_,
spacings_,
gridStyle_] := Module[{
scaledShapes, scaledSeparator, widthWithExtraSeparator, shapesWithExtraSeparator, totalWidth, explicitGridStyle},
gridStyle_] := ModuleScope[
scaledShapes = MapThread[
Scale[
Translate[#1, -#2[[All, 1]]],
Expand Down
4 changes: 3 additions & 1 deletion Kernel/SetReplace.m
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
Package["SetReplace`"]

PackageImport["GeneralUtilities`"]

PackageExport["SetReplace"]

(* This function behaves similarly to StringReplace. The implementation is done with WolframModel, which is a more
Expand Down Expand Up @@ -37,7 +39,7 @@

expr : SetReplace[set_, rules_, events : Except[_ ? OptionQ] : 1, o : OptionsPattern[]] /;
recognizedOptionsQ[expr, SetReplace, {o}] :=
Module[{result},
ModuleScope[
result = Check[
setSubstitutionSystem[rules, set, <|$maxEvents -> events|>, SetReplace, False, o],
$Failed];
Expand Down
4 changes: 3 additions & 1 deletion Kernel/SetReplaceAll.m
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
Package["SetReplace`"]

PackageImport["GeneralUtilities`"]

PackageExport["SetReplaceAll"]

(* The idea for SetReplaceAll is to keep performing SetReplace on the graph until no replacement can be done without
Expand Down Expand Up @@ -29,7 +31,7 @@
expr : SetReplaceAll[
set_, rules_, generations : Except[_ ? OptionQ] : 1, o : OptionsPattern[]] /;
recognizedOptionsQ[expr, SetReplaceAll, {o}] :=
Module[{result},
ModuleScope[
result = Check[
setSubstitutionSystem[
rules, set, <|$maxGenerationsLocal -> generations|>, SetReplaceAll, False, o],
Expand Down
4 changes: 3 additions & 1 deletion Kernel/SetReplaceFixedPoint.m
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
Package["SetReplace`"]

PackageImport["GeneralUtilities`"]

PackageExport["SetReplaceFixedPoint"]

(* Same as SetReplace, but automatically stops replacing when the set no longer changes. *)
Expand All @@ -25,7 +27,7 @@
"EventOrderingFunction" -> Automatic};

SetReplaceFixedPoint[set_, rules_, o : OptionsPattern[]] /;
recognizedOptionsQ[expr, SetReplaceFixedPoint, {o}] := Module[{result},
recognizedOptionsQ[expr, SetReplaceFixedPoint, {o}] := ModuleScope[
result = Check[
setSubstitutionSystem[
rules, set, <||>, SetReplaceFixedPoint, False, o],
Expand Down
4 changes: 3 additions & 1 deletion Kernel/SetReplaceFixedPointList.m
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
Package["SetReplace`"]

PackageImport["GeneralUtilities`"]

PackageExport["SetReplaceFixedPointList"]

(* Same as SetReplaceFixedPoint, but returns all intermediate steps. *)
Expand All @@ -24,7 +26,7 @@
"EventOrderingFunction" -> Automatic};

SetReplaceFixedPointList[set_, rules_, o : OptionsPattern[]] /;
recognizedOptionsQ[expr, SetReplaceFixedPointList, {o}] := Module[{result},
recognizedOptionsQ[expr, SetReplaceFixedPointList, {o}] := ModuleScope[
result = Check[
setSubstitutionSystem[
rules, set, <||>, SetReplaceFixedPointList, False, o],
Expand Down
4 changes: 3 additions & 1 deletion Kernel/SetReplaceList.m
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
Package["SetReplace`"]

PackageImport["GeneralUtilities`"]

PackageExport["SetReplaceList"]

(* Same as SetReplace, but returns all intermediate steps in a List. *)
Expand All @@ -23,7 +25,7 @@

SetReplaceList[set_, rules_, events : Except[_ ? OptionQ] : 1, o : OptionsPattern[]] /;
recognizedOptionsQ[expr, SetReplaceList, {o}] :=
Module[{result},
ModuleScope[
result = Check[
setSubstitutionSystem[rules, set, <|$maxEvents -> events|>, SetReplaceList, False, o],
$Failed];
Expand Down
Loading

0 comments on commit e78a32c

Please sign in to comment.