-
-
Notifications
You must be signed in to change notification settings - Fork 520
fix(package): [slice] Fix RigthPadding and LeftPadding #322
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(package): [slice] Fix RigthPadding and LeftPadding #322
Conversation
|
Do not understand exactly why the check does not work. The build complaints when importing a std lib package. Locally all tests pass. Must the CI/CD include some |
@idichekop, The check failed because the go version in github action was specified to 1.20, which doesn't contain slices package. see screenshot below.
|
Hi, @idichekop. Thanks for your proposal and consideration regarding code modernization. Leveraging the standard library's slices package would indeed enhance code safety, consistency, and lay a solid foundation for optimizing other functions in the future. However, introducing this dependency conflicts with Lancet's current compatibility commitment. As you're aware, Lancet currently supports Go 1.18 and above, while the slices package was officially introduced in Go 1.21. Directly depending on it would break compatibility for users still on Go 1.18-1.20, which goes against our existing support policy. To address this, I'd suggest a compromise: instead of relying on the slices package directly, we could implement equivalent functionality internally for now to maintain compatibility with older Go versions. once Lancet raises its minimum supported Go version to 1.21, we can plan to migrate to the standard slices package. |
That is a fair explanation, Duke! I was not aware of this commitment. I will do as you say. I will submit another version soon. |
Thanks for your understanding, Just send over the revised version when you’re ready. Let me know if you run into any issues. |
|
New proposal submitted. As suggested, I implemented the functionalities inspired by the standard lib. I placed them in the internal package of As I am not yet sure how @duke-git deals with new features and versioning yet, I propose we keep them for the moment as internal. Later, we can transfer the functions and make them accessible if so desired. Cheers, Idichekop |
@idichekop, cool. Lancet's release strategy is a variant of Semantic Versioning, defined as follows:
We can revise the second rule to follow Semantic Versioning: increment the MINOR version when new features are added in a backward-compatible manner. Lancet typically release updates for new features every 2 to 3 months. For critical bug fixes or security-related patches, however, releases will be made immediately. Since these are inspired by standard library patterns and could eventually add value as public utilities, I agree with your proposal. Let’s keep them internal for now but flag them for now, and release in v2.3.8 later. |


The
RightPaddingandLeftPaddingfunctions in theslicepackage have some issues. Also discussed in issue(#320 ):RightPaddingmodifies the given slice in place, whileLeftPaddingreturns a copy of the slice with the padding (or the given slice if the paddingLenght is 0). That is not consistent, and brings problems to the caller as it behaves different depending on the situation.This can be shown by running the following code (or in Play https://go.dev/play/p/y7G86ZQjJBP):
This PR solves:
a. these two issues,
b. modernizes the code,
c. and includes additional tests for padding with negative length, padding empty slices, and padding nil slices.
The catch!
The proposed code uses the standard lib "slices" package for creating the padding suffix/prefix and concatenating the slices. The dependency to the standard lib is already present in the lancet module, BUT was not present in the
slicepackage.I propose we accept this dependency. Since Go1.21 the "slices" package was made official and has many potentials to modernize parts of the current lancet code base. It can be used also to improve other functions (later).
@duke-git , If you think this dependency is not desired, I can re-write the code to mimic it. My point is that some of the functionality on the std lib can be used (also on other functions) to make it safer, more consistent, and similar behavior on corner cases (e.g. dealing with empty slices). The dependency of lancet to the std lib is already accepted and documented.