You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Copy file name to clipboardExpand all lines: coredev/plip-review.md
+82-74Lines changed: 82 additions & 74 deletions
Display the source diff
Display the rich diff
Original file line number
Diff line number
Diff line change
@@ -14,94 +14,102 @@ A Plone Improvement Proposal (PLIP) is a formal process to propose a change to i
14
14
15
15
## Expectations
16
16
17
-
A good PLIP review takes about 4 hours so please plan accordingly.
18
-
When you are done, if you have access to core please commit the review to the PLIPSs folder and reference the PLIP in your commit message.
19
-
If you do not have access, please attach your review to the PLIP ticket itself.
17
+
A good PLIP review takes about four hours.
18
+
Please plan accordingly.
19
+
When you are done, if you have access to core, commit the review to the `plips` folder, and reference the PLIP in your commit message.
20
+
If you do not have access, attach your review to the PLIP ticket itself.
21
+
20
22
21
23
## Setting up the environment
22
24
23
-
Follow the instructions on {ref}`setup-development-environment`.
25
+
Follow the instructions in {doc}`getting-started-with-development`.
24
26
You will need to checkout the branch to which the PLIP is assigned.
25
-
Instead of running the buildout with the default buildout file, you will run the config specific to that PLIP:
27
+
Instead of running the buildout with the default buildout file, you will run the configuration specific to that PLIP:
26
28
29
+
```shell
30
+
./bin/buildout -c plips/plipXXXX.cfg
27
31
```
28
-
> ./bin/buildout -c plips/plipXXXX.cfg
29
-
```
30
32
31
-
## Functionality Review
32
-
33
-
There are several things that could be addressed in a PLIP review depending on the nature of the PLIP itself.
34
-
This is by no means an exhaustive list, but a place to start.
35
-
Things to think about when reviewing:
36
-
37
-
## General
38
-
39
-
- Does the PLIP actually do what the implementers proposed?
40
-
Are there incomplete variations?
41
-
- Were there any errors running buildout?
42
-
Did the migration(s) work?
43
-
- Do error and status messages make sense?
44
-
Are they properly internationalized?
45
-
- Are there any performance considerations?
46
-
Has the implementer addressed them if so?
47
-
48
-
## Bugs
49
-
50
-
- Are there any bugs?
51
-
Nothing is too big nor small.
52
-
- Do fields handle whacky data?
53
-
How about strings in date fields or nulls in required?
54
-
- Is validation up to snuff and sensical?
55
-
Is it too restrictive or not restrictive enough?
56
-
57
-
## Usability Issues
58
-
59
-
- Is the implementation usable?
60
-
- How will novice end users respond to the change?
61
-
- Does this PLIP need a usability review?
62
-
If you think this PLIP needs a usability review,
63
-
please change the state to "please review" and add a note in the comments.
64
-
- Is the PLIP consistent with the rest of Plone?
65
-
For example,
66
-
if there is control panel configuration,
67
-
does the new form fit in with the rest of the panels?
68
-
- Does everything flow nicely for novice and advanced users?
69
-
Is there any workflow that feels odd?
70
-
- Are there any new permissions and do they work properly?
71
-
Does their role assignment make sense?
72
-
73
-
## Documentation Issues
74
-
75
-
- Is the corresponding documentation for the end user,
76
-
be it developer or plone user,
77
-
sufficient?
33
+
34
+
## Functionality review
35
+
36
+
This section describes the topics that may be addressed in a PLIP review, depending on the nature of the PLIP itself.
37
+
38
+
39
+
### General
40
+
41
+
- Does the PLIP actually do what the implementers proposed?
42
+
Are there incomplete variations?
43
+
- Were there any errors running buildout?
44
+
Did the migration(s) work?
45
+
- Do error and status messages make sense?
46
+
Are they properly internationalized?
47
+
- Are there any performance considerations?
48
+
Has the implementer addressed them, if so?
49
+
50
+
51
+
### Bugs
52
+
53
+
- Are there any bugs?
54
+
Nothing is too big nor small.
55
+
- Do fields handle wacky data?
56
+
How about strings in date fields, or nulls in required?
57
+
- Is validation up to snuff and sensical?
58
+
Is it too restrictive or not restrictive enough?
59
+
60
+
61
+
### Usability Issues
62
+
63
+
- Is the implementation usable?
64
+
- How will novice end users respond to the change?
65
+
- Does this PLIP need a usability review?
66
+
If you think this PLIP needs a usability review, change the state to "please review" and add a note in the comments.
67
+
- Is the PLIP consistent with the rest of Plone?
68
+
For example, if there is control panel configuration, does the new form fit in with the rest of the panels?
69
+
- Does everything flow nicely for novice and advanced users?
70
+
Is there any workflow that feels odd?
71
+
- Are there any new permissions and do they work properly?
72
+
Does their role assignment make sense?
73
+
74
+
75
+
### Documentation Issues
76
+
77
+
- Is the corresponding documentation for the end user, be it developer or Plone user, sufficient?
78
78
- Is the change itself properly documented?
79
79
80
-
Please report bugs/issues on GitHub as you would for any Plone bug.
81
-
Reference the PLIP in the bug, assign to its implementer, and add a tag for the PLIP in the form of plip-xxx.
82
-
This way the implementer can find help if he needs it.
83
-
Please also prioritize the ticket.
80
+
Report bugs or issues on GitHub as you would for any Plone bug.
81
+
Reference the PLIP in the bug, assign to its implementer, and add a tag for the PLIP in the form of `plip-xxx`.
82
+
This way the implementer can find help if they need it.
83
+
Also set a priority for the ticket.
84
84
The PLIP will not be merged until all blockers and critical bugs are fixed.
85
85
86
-
## Code Review
87
86
88
-
### Python
87
+
### Code Review
89
88
90
-
- Is this code maintainable?
91
-
- Is the code properly documented?
92
-
- Does the code adhere to PEP8 standards (more or less)?
93
-
- Are they importing deprecated modules?
94
89
95
-
###JavaScript
90
+
#### Python
96
91
97
-
- Does the JavaScript meet our set of JavaScript standards?
98
-
See our section about {doc}`JavaScript </develop/addons/javascript/index>` and the {doc}`JavaScript styleguide </develop/styleguide/javascript>`.
99
-
- Does the JavaScript work in all currently supported browsers?
100
-
Is it performant?
92
+
- Is this code maintainable?
93
+
- Is the code properly documented?
94
+
- Does the code adhere to PEP8 standards (more or less)?
95
+
- Are they importing deprecated modules?
96
+
97
+
98
+
#### JavaScript
99
+
100
+
- Does the JavaScript meet our set of JavaScript standards?
101
+
See our section about [JavaScript](https://5.docs.plone.org/develop/addons/javascript/index.html) and the [JavaScript Style Guide](https://5.docs.plone.org/develop/styleguide/javascript.html).
102
+
- Does the JavaScript work in all currently supported browsers?
103
+
Is it performant?
104
+
105
+
```{todo}
106
+
Update links from Plone 5 Documentation to Plone 6 Documentation, when they exist.
107
+
See https://github.com/plone/documentation/issues/1330
108
+
```
101
109
102
-
### ME/TAL
110
+
####ME/TAL
103
111
104
-
- Does the PLIP use views appropriately and avoiding too much logic?
105
-
- Is there any code in a loop that could potentially be a performance issue?
106
-
- Are there any deprecated or old style ME/TAL lines of code such as using DateTime?
107
-
- Is the rendered html standards compliant? Are ids and classes used appropriately?
112
+
-Does the PLIP use views appropriately, avoiding too much logic?
113
+
-Is there any code in a loop that could potentially be a performance issue?
114
+
-Are there any deprecated or old style ME/TAL lines of code, such as using `DateTime`?
115
+
-Is the rendered HTML compliant with standards? Are IDs and classes used appropriately?
0 commit comments