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: docs/contributing/core/plip-review.md
+37-43Lines changed: 37 additions & 43 deletions
Display the source diff
Display the rich diff
Original file line number
Diff line number
Diff line change
@@ -10,107 +10,101 @@ myst:
10
10
# PLIP review
11
11
12
12
A Plone Improvement Proposal (PLIP) is a formal process to propose a change to improve Plone.
13
+
A review of a PLIP ensures that proposed behavior has been implemented as expected.
13
14
15
+
Post your PLIP review as a comment in the PLIP pull request.
14
16
15
-
## Expectations
16
17
17
-
A good PLIP review takes about four hours.
18
-
Please plan accordingly.
19
-
20
-
When you are done, if you have access to core, commit the review to the `plips` folder of [`buildout.coredev`](https://github.com/plone/buildout.coredev), and reference the PLIP in your commit message.
21
-
If you do not have access, attach your review to the PLIP ticket itself.
22
-
23
-
24
-
## Setting up the environment
18
+
## Set up the environment
25
19
26
20
Follow the instructions in {doc}`index`.
27
21
You will need to checkout the branch to which the PLIP is assigned.
28
22
Instead of running the buildout with the default buildout file, you will run the configuration specific to that PLIP:
This section describes the topics that may be addressed in a PLIP review, depending on the nature of the PLIP itself.
38
32
33
+
Report bugs or issues you find during the review on GitHub as you would for any Plone bug.
34
+
Reference the PLIP in the bug, assign it to its implementer, and add a tag for the PLIP in the form of `plip-xxx`.
35
+
This way the implementer can find help if they need it.
36
+
Also set a priority for the ticket.
37
+
The PLIP will not be merged until all blockers and critical bugs are fixed.
38
+
39
39
40
40
### General
41
41
42
+
- Take screenshots or videos as necessary, and post them to the pull request with your comments.
42
43
- Does the PLIP actually do what the implementers proposed?
43
-
Are there incomplete variations?
44
+
- Are there incomplete variations?
44
45
- Were there any errors running buildout?
45
-
Did the migration(s) work?
46
+
-If there were migrations, did they work?
46
47
- Do error and status messages make sense?
47
-
Are they properly internationalized?
48
+
- Are messages properly internationalized?
48
49
- Are there any performance considerations?
49
-
Has the implementer addressed them, if so?
50
50
51
51
52
52
### Bugs
53
53
54
54
- Are there any bugs?
55
-
Nothing is too big nor small.
56
-
- Do fields handle wacky data?
57
-
How about strings in date fields, or nulls in required?
58
-
- Is validation up to snuff and sensical?
59
-
Is it too restrictive or not restrictive enough?
55
+
- Do fields handle unexpected data?
56
+
- Is validation function correctly and make sense?
57
+
- Is validation too restrictive or not restrictive enough?
60
58
61
59
62
-
### Usability Issues
60
+
### Usability issues
63
61
64
62
- Is the implementation usable?
65
-
- How will novice end users respond to the change?
63
+
- How will end users respond to the change?
66
64
- Does this PLIP need a usability review?
67
-
If you think this PLIP needs a usability review, change the state to "please review" and add a note in the comments.
65
+
If you think this PLIP needs a usability review, add GitHub issue labels {guilabel}`99 tag: UX`, or similar, and {guilabel}`32 needs: review`, and add a note in the comments.
68
66
- Is the PLIP consistent with the rest of Plone?
69
67
For example, if there is control panel configuration, does the new form fit in with the rest of the panels?
70
-
- Does everything flow nicely for novice and advanced users?
71
-
Is there any workflow that feels odd?
68
+
- Does everything flow nicely for all users?
72
69
- Are there any new permissions and do they work properly?
73
-
Does their role assignment make sense?
70
+
- Does permission assignment to roles make sense?
74
71
75
72
76
-
### Documentation Issues
73
+
### Documentation issues
77
74
78
-
- Is the corresponding documentation for the end user, be it developer or Plone user, sufficient?
79
-
- Is the change itself properly documented?
75
+
-Is the corresponding documentation for the end user, be it developer or Plone user, sufficient?
76
+
-Is the change itself properly documented?
80
77
81
-
Report bugs or issues on GitHub as you would for any Plone bug.
82
-
Reference the PLIP in the bug, assign to its implementer, and add a tag for the PLIP in the form of `plip-xxx`.
83
-
This way the implementer can find help if they need it.
84
-
Also set a priority for the ticket.
85
-
The PLIP will not be merged until all blockers and critical bugs are fixed.
86
78
79
+
## Code review
87
80
88
-
### Code Review
81
+
This section describes considerations of code quality.
89
82
90
83
91
-
####Python
84
+
### Python
92
85
93
86
- Is this code maintainable?
94
87
- Is the code properly documented?
95
-
- Does the code adhere to PEP8 standards (more or less)?
96
-
- Are they importing deprecated modules?
88
+
- Does the code adhere to formatting and style standards?
89
+
- Are there any importied deprecated modules?
97
90
98
91
99
-
####JavaScript
92
+
### JavaScript
100
93
101
-
- Does the JavaScript meet our set of JavaScript standards?
102
-
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).
94
+
- Does the JavaScript satisfy the package's JavaScript standards, or if the package has no standard, then Plone's?
95
+
See Plone's section about {doc}`plone5:develop/addons/javascript/index` and the {doc}`plone5:develop/styleguide/javascript`.
103
96
- Does the JavaScript work in all currently supported browsers?
104
-
Is it performant?
97
+
- Is the JavaScript performant?
105
98
106
99
```{todo}
107
100
Update links from Plone 5 Documentation to Plone 6 Documentation, when they exist.
108
101
See https://github.com/plone/documentation/issues/1330
109
102
```
110
103
111
-
####ME/TAL
104
+
### ME/TAL
112
105
113
106
- Does the PLIP use views appropriately, avoiding too much logic?
114
107
- Is there any code in a loop that could potentially be a performance issue?
115
108
- Are there any deprecated or old style ME/TAL lines of code, such as using `DateTime`?
116
-
- Is the rendered HTML compliant with standards? Are IDs and classes used appropriately?
0 commit comments