Motivation:
With the release of Swift 5.1, the official docker containers are available for both 16.04 and 18.04 of Linux - so the test matrix should include them.
Modifications:
- Add new 5.1 jobs for 18.04 linux and change 16.04 5.1 jobs to use the official docker images.
- Update the README to capture the changes
Result:
The number of configurations the library is tested against should be as wide as possible for regular interval testing passes.
Motivation:
While working to add more test coverage with `RESPTranslator`, it was made apparent that a `.bulkString(.none)` is impossible to create directly with the `RESPValue` initializers, even though it is a reasonable possibility.
Additionally, forcing all integer types to have to be stored in an `Int` is unnecessarily restrictive.
Modifications:
- Change `RESPValue.init(bulk:)` initializers to accept `Optional` instances
- Change `RESPValue.init(bulk:)` for `Int` initializer to be generic on `FixedWidthInteger`
Result:
Converting types to and from `RESPValue` should be more bi-directional and seamless.
Motivation:
Diagnostics for why `.bulkString` parses might fail were weak, and edge cases fell through gaps in coverage were found.
Modifications:
Added new cases to `RESPTranslator.ParsingError` for `.bulkString` parsing with additional test coverage.
Result:
Users should have better diagnostics for bogus data or failed parsing state.
Motivation:
While working on issue #56, it was forgotten to add the new test cases to the linux manifest file.
Modifications:
Update the linux manifests to include all current unit tests
Result:
All written unit tests should be ran on Linux
Motivation:
MRs take unnecessarily too long to have completed pipelines with "redundant" builds happening in sequence.
The entire stage of building in release is an additional QA layer that can happen on merges into master, but for MRs are just added CPU cycles.
While it's useful to ensure that building in release is possible, we will get almost all build failures from the test stage, and the most important checks are the unit and integration tests.
Modifications:
Change the ordering of build vs. test, as well as only running "build in release" jobs on the master branch.
Result:
MRs should be able to be merged sooner as CI pipelines take less time to run.
Motivation:
After working with RedisKit with RediStackTestUtils as a dependency, it was realized how opinionated the module is in how RedisConnections can be created in test environments.
Modifications:
Require more information, with reasonable defaults for `RedisConnection.init()`. Provide subclass hooks for `RedisIntegrationTestCase` for implementors to make decisions for themselves at how to connect to Redis.
Result:
Users should have more freedom in how they connect to Redis in their units tests.
Motivation:
For users looking to contribute, and for those looking to validate the library, it was unclear what tests require an actual connection to a Redis instance in order to run.
Modifications:
Add a `RediStackIntegrationTests` that takes all tests that require a Redis instance in order to run.
Result:
Those looking to run just unit tests, or contribute new tests, can now directly point to a specific testTarget as defined in the Package manifest.
Motivation:
There is a reasonable way to compare if two `RedisErrors` are equal, which was seen as needed in the `Equatable` conformance for `RESPValue`.
Modifications:
Added `Equatable` conformance for `RedisError` by comparing the messages.
Result:
Two `RedisError` instances are now equatable.
Motivation:
While working on unit tests the need for conformance to `Equatable` for `RESPValue` has been needed a few times and it was decided to make it public.
Modifications:
Added conformance to `Equatable` for `RESPValue` with unit test.
Result:
Users should now be able to compare two `RESPValue` instances for equality.
Motivation:
During the refactor work (commit ea7c755) that was merged with MRs !71 and !53 - `sendCommandsImmediately` was accidentally lowered to `internal`
Modifications:
Properly mark `RedisConnection.sendCommandsImmediately` as `public`
Result:
Developers should now have proper access to the `sendCommandsImmediately` property
Motivation:
A handful of times, unit tests were forgotten to be added to the `allTests` extension, or were incorrectly copy/pasted.
Modifications:
Remove manual entries of `allTest` and use generated result from `swift test --generate-linuxmain`
Result:
There should be proper test parity between macOS and Linux.
Motivation:
As this project might take off and support be needed, users will want a direct line of communication to file support tickets.
Modifications:
Update docs to include references and links for contacting the RediStack maintainers for assistance.
Result:
Users should have more information on how to contact project maintainers.
Motivation:
Issue #60 called for improving the type safety of the options available for the `zadd` command, and MR !70 made some great headway, but attempted to cram too much into a single enum.
Modifications:
- Break the `RedisSortedSetAddOption.returnChangedCount` value into an additional boolean param
Result:
Using `zadd` should now be more straight forward, while being type safe.
Motivation:
There are several cases where "command" could refer to a command keyword, or an entire message (keyword + args). This made working with `RedisCommand` and it's documentation ambiguous.
Modifications:
- Rename `RedisCommand.command` to `message`
- Rename initializer labels to `message` and `responsePromise`
Result:
When encountering a `RedisCommand` everyone should know that they are dealing with a message that should be sent to Redis as soon as possible.
Motivation:
To give a better first impression, and providing more comprehensive information for first time viewers, the README file needed reworking.
Modifications:
- Add a CHANGELOG file that at least redirects readers to the releases page of GitLab
- Update the CONTRIBUTORS list
- Change project README to have more information, flow better, and to have a better "first impression"
Result:
The README should give a stronger indication into the project's health.
Motivation:
Up until now, Jazzy docs were generated on a private CI runner hosting macOS - which doesn't make CI portable as it requires that runner to always be available.
Modifications:
- Change: CI config to use a Linux Jazzy Docker image for portability
- Change: Doc theme templates to not reference GitHub
Result:
Jazzy docs should be able to be generated at any time, as the CI infrastructure is using GitLab's cloud solution with a Docker image
Motivation:
To make it a little more generic, and to avoid turnover during renames (such as the planned rebranding in issue #61), `RedisClientError` more accurately reflects the source of the errors, as well as the responsibility of causing the bug.
Modifications:
- Rename `RedisNIOError` to `RedisClientError`
- Rename `RedisError` file to `RedisErrors`
- Add documentation of `RedisClientError`
- Remove no longer used `.unsupportedOperation(method:message:)` value
- Rename `.responseConversion(to:)` to `.failedRESPConversion(to:)`
Result:
Names of `RedisClientError` should be more descriptive, less prone to turnover, and more documented for users to understand the issues related to these thrown errors.
Motivation:
During proposal review, and while working within the codebase, several issues were identified with how `RedisConnection` was architectured.
Modifications:
- Change implementation of `RedisConnection` in some areas for new logic of internal `ConnectionState`
- Change behavior of logging in a few places
- The initializer for `RedisConnection` is now **internal**
- How users can override the default `ClientBootstrap` for a connection is by passing an instance to the `.connect` static method
- Change unit tests to inherit from a common XCTestCase class that handles creation and cleanup of `RedisConnection` in tests
- Remove Redis namespace enum
Result:
The API for `RedisConnection` should be much simpler, with the implementation being less buggy.
This resolves issues #49, #54, and #57.
Motivation:
While working through issue #59, it was noticed just how "stringly" the SortedSet command options for `zadd`, `zinterstore`, and `zunionstore` were, and Swift provides ways of having strong type safety for these options.
Modifications:
- Add `RedisSortedSetAddOption` and `RedisSortedSetAggregateMethod` to replace the String API in `zadd`, `zinterstore`, and `zunionstore`
- Fix an implication of how `overestimatedCountBeingAdded` documentation for `Array where Element == RESPValue` for `add(contentsOf:overestimatedCountBeingAdded:_:)`
Result:
Users should have a more discoverable and straightforward way that isn't error prone for calling `zadd`, `zinterstore`, and `zunionstore` with Redis supported options.
Motivation:
Johannes continues to provide great insight, and correctly pointed out that `RESPValueConvertible` was being used as an "existential" in all cases.
This can cause unexpected type-erasure and introduce unnecessary cost overhead with dynamic dispatch when in most cases we know the exact value we want for `RESPValue` to execute commands.
Modifications:
- Add new extensions to `Array where Element == RESPValue` for appending and adding elements into them
- Change `RedisClient.send(command:with:)` to require `[RESPValue]` instead of `[RESPValueConvertible]` as the `with` argument type
- Change all instances of `RESPValueConvertible` being an "existential" type for method arguments to instead be a generic constraint
Result:
The library should be safeguarded from a class of bugs, with the use of `send` being a bit more straight forward, with some new convenience methods for `[RESPValue]` types.
Motivation:
Johannes provided a fair code review of the project and summarized his findings in issue #48, and one of the prime offenders was all of the `unsafe*` APIs (pointers, buffers, bytes)
that were used with `RESPValue` and `RESPValueConvertible`.
He also provided great feedback and pointed out good points of confusion with the API design of `RESPValue` and `RESPValueConvertible`.
Modifications:
- Return to using `Array` instead of `ContiguousArray` for `RESPValue.array` storage
- Update all documentation to be more thorough in explaining how the types should be used and conformed to.
- Remove all uses of `unsafe*` APIs where possible
- Change implementations to be a lot more type and memory safe, double checking assumptions
- Remove conformance to `ExpressibleBy*Literal` as it is too easy for users to shoot themselves in the foot and saves only a few characters over `.init(bulk:)`
- Create new `RedisNIOTestUtils` target for common test extensions, making them public
- Move most almost all implementations of `RESPValue` computed properties into the `RESPValueConvertible` conformances
Result:
Users should be more safeguarded by the API against unknowingly getting incorrect `RESPValue` representations, the API design of `RESPValue` and `RESPValueConvertible` should be much clearer,
and memory safety should be at a higher bar from these changes.
This resolves issues #55 & #48, and contributes to issue #47.
Motivation:
During proposal review, it was noted that `RedisCommandContext` was a bit misleading, and the hidden reference semantics worrisome.
In addition, several of parts of the documentation around `RedisCommandHandler` were weak or also misleading.
Modifications:
- Rename `RedisCommandContext` to just `RedisCommand`
- Update documentation to be more explicit about the module who owns the types being referenced
- Update documentation to call out explicit usage semantics and behavior
- Change `RedisCommandHandler` to close the socket connection on error thrown
- Rename the "base" Redis Channel Handlers to be more explicitly named
Result:
Users should have clearer documentation on what happens when using `RedisCommandHandler` and `RedisCommand` without hidden semantics.
This contributes to issue #47.
Motivation:
During proposal review, it was appropriately pointed out that `RedisClient.expire` incorrectly mixes 'deadline' and 'timeout' terminology.
Modifications:
- Change references of 'deadline' to 'timeout' to follow Redis' established semantics for 'EXPIRE'
- Add additional unit test for `RedisClient.expire`
Result:
`RedisClient.expire` should now be more clear as to its semantics and not mix terminology incorrectly.
This contributes to issue #47
Motivation:
During proposal review, feedback was provided that the discoverability of the factory method for building a standard RESP `ChannelPipeline` was poor outside of documentation.
Modifications:
- Move `Redis.makeDefaultClientBootstrap` to `ClientBootstrap.makeRedisTCPClient`.
- Move the `channelInitializer` implementation into a new `Channel.addBaseRedisHandlers()` instance method.
Result:
Users should have an easier time discovering how to easily create baseline RESP `ChannelPipelines`.
This contributes to #47.
Nathan Harris