on eb16544 help sign in
streamly/CONTRIBUTING.md
251 lines 11 kB history raw
1# Contributors' Guide
3## Bug Reports
5Please feel free to [open an
6issue](https://github.com/composewell/streamly/issues) for any questions,
7suggestions, issues or bugs in the library. In case you are reporting a bug
8we will appreciate if you provide as much detail as possible to reproduce or
9understand the problem, but nevertheless you are encouraged to open an issue
10for any problem that you may encounter.
12## Picking Issues to Work on
14Beginners are encouraged to pick up issues that are marked `help wanted`. It is
15a good idea to update the issue expressing your intent so that others do not
16duplicate the effort and people with a background on the issue can help.
18## Contributing A Change
20If the feature makes significant changes to design, we encourage you to open an
21issue as early as possible so that you do not have to redo much work because of
22changes in design decisions. However, if you are confident, you can still go
23ahead and take that risk as the maintainers are supposed to be reasonable
24people.
26### Pull Requests (PR)
28Please feel free to [send a pull
29request (PR)](https://github.com/composewell/streamly/pulls) whether it is a
30single letter typo fix or a complex change. We will accept any PR that makes a
31net positive change to the package. We encourage you to provide a complete,
32consistent change with test, documentation and benchmarks. You can contact the
33[maintainers](https://gitter.im/composewell/streamly) for any help or
34collaboration needed in that regard. However, if due to lack of time you are
35not able to complete the PR, you are still welcome to submit it, maintainers
36will try their best to actively contribute and pick up your change as long as
37the change is approved.
39### Pull Request (PR) Checklist
41Here is a quick checklist for a PR, for details please see the next section:
43* PR contains one logical changeset
44* Each commit in the PR consists of a logical change
45* Commits are rebased/squashed/fixup/reordered as needed
46* Stylistic changes to irrelevant parts of the code are separated in
47 independent commits.
48* Code is formatted as per the style of the file or that of other files
49* Compiler warnings are fixed
50* Reasonable hlint suggestions are accepted
51* Tests are added to cover the changed parts
52* All [tests](test) pass
53* [Performance benchmarks](benchmark) are added, where applicable
54* No significant regressions are reported by [performance benchmarks](benchmark/README.md)
55* Haddock documentation is added to user visible APIs and data types
56* Tutorial module, [README](README.md), and [guides](docs) are updated if
57 necessary.
58* [Changelog](Changelog.md) is updated if needed
59* The code conforms to the license, it is not stolen, credit is given,
60 copyright notices are retained, original license is included if needed.
62### Structuring Pull Requests
64__Lean PR:__ Please keep the reviewer in mind when sending pull
65requests. Use one PR for each logical changeset. A lean and thin
66PR with one independent logical change is more likely to be reviewed and
67merged quickly than a big monolithic PR with several unrelated changes.
69__PR Dependencies:__ If your change depends on an earlier PR you can
70create a branch from the old PR's branch, and raise a new PR targeting
71to merge into the old PR branch. Do not push the commits to the same PR
72just because the commit depends on that PR.
74__Commits:__ You are encouraged to group a logically related set of
75changes into a single commit. When the overall changeset is largish you
76can divide it into multiple smaller commits, with each commit having
77a logically grouped changeset and the title summarizing the logical
78grouping. Always keep in mind a logical division helps reviewers
79understand and review your code quickly, easier history tracking and
80when required clean reversal changes.
82__Functional Changes:__ Keep the reviewer in mind when making
83changes. Please resist the temptation to make style related changes to
84surrounding code unless you are changing that code anyway . Whenever
85possible, try to separate unrelated refactoring changes which do not
86affect functionality in separate commits so that it is easier for the
87reviewer to verify functional changes.
89__Style Changes:__ Make sure that your IDE/Editor is not automatically
90making sweeping style changes to all the files you are editing. That
91makes separating the signal from the noise very difficult and makes
92everything harder. If you would like to make style related changes
93then please send a separate PR with just those changes and no other
94functionality changes.
96__Rebasing:__ If your commits reflect how you fixed intermediate
97problems during testing or made random changes at different times you
98may have to squash your changes (`git rebase -i`) into a single commit
99or logically related set of commits.
101### Resolving Conflicts
103If during the course of development or before you send the PR you find that
104your changes are conflicting with the master branch then use `git rebase
105master` to rebase your changes on top of master. DO NOT MERGE MASTER INTO YOUR
106BRANCH.
108### Testing
110It is a good idea to include tests for the changes where applicable. See the
111existing tests [here](test).
113### Documentation
115For user visible APIs, it is a good idea to provide haddock documentation that
116is easily understood by the end programmer and does not sound highfalutin,
117and preferably with examples. If your change affects the tutorial or needs to
118be mentioned in the tutorial then please update the tutorial. Check if the
119additional [guides](docs) are affected or need to updated.
121### Performance Benchmarks
123It is a good idea to run performance benchmarks to see if your change affects
124any of the existing performance benchmarks. If you introduced something new
125then you may want to add benchmarks to check if it performs as well as expected
126by the programmers to deem it usable.
128See the [README](benchmark/README) file in the `benchmark` directory for more
129details on how to run the benchmarks.
131The first level of check is the regression in "allocations", this is
132the most stable measure of regression. "bytesCopied" is another stable
133measure, it gives an idea of the amount of long lived data being copied
134across generations of GC.
136The next measure to look at is "cpuTime" this may have some variance
137from run to run because of factors like cpu frequency scaling, load on
138the system or the number of context switches etc. However, the variance
139would usually be within 5-10%, anything more than that is likely to be a
140red flag.
142Quite often an increase in "cpuTime" would correspond to an increase
143in "allocations". Increase in "allocations" indicates an inefficiency
144in computing due to too much GC activity e.g. due to lack of
145fusion. However, it is also possible for the cpu time to go up without
146the allocations going up, this indicates an inefficiency in processing
147in general or more CPU being used for the same task. It could be because
148of inefficient code generation, branch mis-predictions or lack of cache
149locality etc.
151For concurrent benchmarks we can compare "cpuTime" and "time" to check
152the degree of concurrency and total efficiency.
154### Changelog
156Any new changes that affect the user of the library in some way must be
157documented under `Unreleased` section at the top of the `Changelog`. The
158changes in the changelog must be organized in the following categories, in that
159order:
161* Breaking Changes
162* Enhancements
163* Bug Fixes
164* Deprecations
166If there are very few changes then you can just prefix a bullet with these
167annotations. If there are many changes make sections to group them. A section
168can be absent if there is nothing to add in it.
170If you make changes that are incompatible with the released versions
171of the library please indicate that in the `Changelog` as `Breaking Changes`
172and also write short notes regarding what the programmers need to do to adapt
173their existing code to the new change.
175### Licensing
177If you have copied code from elsewhere you need to conform to the
178licensing terms of the original code. Usually you would need to add a
179copyright notice in the source header, and include the license of the
180original code as per the terms of the license. If the code you are
181copying does not have an associated license please do not use it.
183## Developer documentation
185To build haddock documentation for all modules, including the ones not exposed
186to users of the library, use the following commands:
188```
189$ cabal haddock
190$ stack haddock --no-haddock-deps
191```
193For design documentation see the `design` directory.
195## Coding Guidelines
197### Coding Style
199Please see [the Haskell coding style guide](https://github.com/composewell/haskell-dev/blob/master/coding-style.rst).
201### StreamD coding style
203Some conventions that we follow in the StreamD code are illustrated by the
204following example:
206```
207mapM f (Stream step1 state1) = Stream step state
208 where
210 step gst st = do
211 r <- step1 (adaptState gst) st
212 case r of
213 Yield x s -> f x >>= \a -> return $ Yield a s
214 Skip s -> return $ Skip s
215 Stop -> return Stop
216```
218* For the input streams use numbering for `step` and `state` e.g.
219 step1/state1. For the output stream use `step` and `state`.
220* For state argument of `step`, use `st`.
221* For result of executing a `step` use `r` or `res`
222* For the yielded element use `x`
223* For the yielded state use `s`
225In general, the rule is - the shorter the scope of a variable the shorter its
226name can be. For example, `s` has the shortest scope in the above code, `st`
227has a bigger scope and `state` has the biggest scope.
229## Design guides
231See [the design directory](design) for design guidelines and documents.
233### Tricky Parts
235The state-passing through each API is currently fragile. Every time we run a
236stream we need to be careful about the state we are passing to it. In case of
237folds where there is no incoming state, we start with the initial state
238`defState`. When we have an incoming state passed to us there are two cases:
2401. When we are building a concurrent stream that needs to share the same `SVar`
241 we pass the incoming state as is.
2422. In all other cases we must not share the SVar and every time we pass on the
243 state to run a stream we must use `adaptState` to reset the `SVar` in the
244 state.
246When in doubt just use `adaptState` on the state before passing it on, we will at
247most lose concurrency but the behavior will be correct.
249There is no type level enforcement about this as of now, and therefore we need
250to be careful when coding. There are specific tests to detect and report any
251problems due to this, all transform operations must be added to those tests.