You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@devlake.apache.org by "d4x1 (via GitHub)" <gi...@apache.org> on 2023/03/14 15:18:17 UTC
[GitHub] [incubator-devlake] d4x1 opened a new pull request, #4662: refactor: remove depracted `rand.Seed`
d4x1 opened a new pull request, #4662:
URL: https://github.com/apache/incubator-devlake/pull/4662
### ⚠️ Pre Checklist
> Please complete _ALL_ items in this checklist, and remove before submitting
- [x] I have read through the [Contributing Documentation](https://devlake.apache.org/community/).
- [ ] I have added relevant tests.
- [ ] I have added relevant documentation.
- [x] I will add labels to the PR, such as `pr-type/bug-fix`, `pr-type/feature-development`, etc.
<!--
Thanks for submitting a pull request!
We appreciate you spending the time to work on these changes.
Please fill out as many sections below as possible.
-->
### Summary
rand.Seed is deprecated, more detail https://pkg.go.dev/math/rand#Seed
### Does this close any open issues?
No
### Screenshots
No need
### Other Information
No need
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] d4x1 commented on pull request #4662: refactor: remove depracted `rand.Seed`
Posted by "d4x1 (via GitHub)" <gi...@apache.org>.
d4x1 commented on PR #4662:
URL: https://github.com/apache/incubator-devlake/pull/4662#issuecomment-1471382510
Agreed! These two functions do the same thing. Of course them can be merged and moved to `core/utils`.
But after doing that, I think there are some options to deal with it.
1. without `init` function where we can do `rand.Seed` in, and use `rand.Int` directly in `RandSomeStr`. this will change the original behavior.
2. without `init` function where we can do `rand.Seed` in, and use `rand.New(rand.NewSource)` in `RandSomeStr`. This will be inefficient.
3. add `init` function, we can do `rand.Seed` in it, and `rand.Int` in `RandSomeStr`, add some comments.
I think 1 and 2 are the right way. 3 is temporary and outdated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] warren830 commented on pull request #4662: refactor: remove depracted `rand.Seed`
Posted by "warren830 (via GitHub)" <gi...@apache.org>.
warren830 commented on PR #4662:
URL: https://github.com/apache/incubator-devlake/pull/4662#issuecomment-1469193630
BTW, are you already in our wechat group? @d4x1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] klesh commented on pull request #4662: refactor: remove depracted `rand.Seed`
Posted by "klesh (via GitHub)" <gi...@apache.org>.
klesh commented on PR #4662:
URL: https://github.com/apache/incubator-devlake/pull/4662#issuecomment-1471424591
@d4x1
Yes, I was saying that we should opt for option 2 for now because it is not a true bottleneck of our system.
We may opt for 1 in the future after we upgrade to go v1.20 .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] warren830 commented on pull request #4662: refactor: remove depracted `rand.Seed`
Posted by "warren830 (via GitHub)" <gi...@apache.org>.
warren830 commented on PR #4662:
URL: https://github.com/apache/incubator-devlake/pull/4662#issuecomment-1469193157
Nice work! I left a comment, let me know whenever you need any help
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] d4x1 commented on a diff in pull request #4662: refactor: remove depracted `rand.Seed`
Posted by "d4x1 (via GitHub)" <gi...@apache.org>.
d4x1 commented on code in PR #4662:
URL: https://github.com/apache/incubator-devlake/pull/4662#discussion_r1136473339
##########
backend/server/services/notification.go:
##########
@@ -22,17 +22,17 @@ import (
"encoding/hex"
"encoding/json"
"fmt"
- "github.com/apache/incubator-devlake/core/errors"
- "github.com/apache/incubator-devlake/core/models"
"io"
"math/rand"
"net/http"
"strings"
"time"
+
+ "github.com/apache/incubator-devlake/core/errors"
+ "github.com/apache/incubator-devlake/core/models"
)
func init() {
Review Comment:
Of course it should be deleted, I have made a mistake.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] d4x1 commented on pull request #4662: refactor: remove depracted `rand.Seed`
Posted by "d4x1 (via GitHub)" <gi...@apache.org>.
d4x1 commented on PR #4662:
URL: https://github.com/apache/incubator-devlake/pull/4662#issuecomment-1483020871
I will try to do like this in other PRs. It will not take too long when I become more familiar with the whole project.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] klesh commented on pull request #4662: refactor: remove depracted `rand.Seed`
Posted by "klesh (via GitHub)" <gi...@apache.org>.
klesh commented on PR #4662:
URL: https://github.com/apache/incubator-devlake/pull/4662#issuecomment-1471260795
> > Accroding to https://pkg.go.dev/math/rand#Seed
> > ```
> > 1. The function is deprecated from go v1.20. but we are at v1.19 https://github.com/apache/incubator-devlake/blob/main/backend/go.mod#L3 . We shouldn't remove it unless we upgrade to v1.20
> >
> > 2. If we do so, I think the only thing we need to do is remove the `init` function since it will be seeded randomly automatically.
> > ```
>
> Although it was marked deprecated since 1.20, `rand.New()` can work in 1.19 https://pkg.go.dev/math/rand@go1.19#New . So this change is safe .I can see some cases in main branch https://github.com/apache/incubator-devlake/blob/main/backend/core/plugin/plugin_utils.go#LL148
>
> As for the semantics, according to the comments in `rand.Seed`, `rand.New(source)` is equivalent to `rand.Seed && rand.Int`, just like this PR did.
>
> Also we can use `rand.Int`to produce random integer, IMO, this two ways are equivalent. The both works.
Creating and seeding a new `Rand` instance on each call looks inefficient.
However, I understand your point. I suggest that we do the following:
1. these 2 functions are doing the same thing, we should merge them and move it to the `core/utils` package
2. we can create and seed a new `Rand` at this point, but we should also add a comment to declare that it can be optimized if we upgrade to go v1.20
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] warren830 commented on a diff in pull request #4662: refactor: remove depracted `rand.Seed`
Posted by "warren830 (via GitHub)" <gi...@apache.org>.
warren830 commented on code in PR #4662:
URL: https://github.com/apache/incubator-devlake/pull/4662#discussion_r1136459591
##########
backend/server/services/notification.go:
##########
@@ -22,17 +22,17 @@ import (
"encoding/hex"
"encoding/json"
"fmt"
- "github.com/apache/incubator-devlake/core/errors"
- "github.com/apache/incubator-devlake/core/models"
"io"
"math/rand"
"net/http"
"strings"
"time"
+
+ "github.com/apache/incubator-devlake/core/errors"
+ "github.com/apache/incubator-devlake/core/models"
)
func init() {
Review Comment:
I think you can also delete this function
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] d4x1 commented on pull request #4662: refactor: remove depracted `rand.Seed`
Posted by "d4x1 (via GitHub)" <gi...@apache.org>.
d4x1 commented on PR #4662:
URL: https://github.com/apache/incubator-devlake/pull/4662#issuecomment-1471209768
> Accroding to https://pkg.go.dev/math/rand#Seed
>
> 1. The function is deprecated from go v1.20. but we are at v1.19 https://github.com/apache/incubator-devlake/blob/main/backend/go.mod#L3 . We shouldn't remove it unless we upgrade to v1.20
>
> 2. If we do so, I think the only thing we need to do is remove the `init` function since it will be seeded randomly automatically.
Although it was marked deprecated since 1.20, `rand.New()` can work in 1.19 https://pkg.go.dev/math/rand@go1.19#New . So this change is safe .I can see some cases in main branch https://github.com/apache/incubator-devlake/blob/main/backend/core/plugin/plugin_utils.go#LL148
As for the semantics, according to the comments in `rand.Seed`, `rand.New(source)` is equivalent to `rand.Seed && rand.Int`, just like this PR did.
Also we can use `rand.Int`to produce random integer, IMO, this two ways are equivalent. The both works.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] d4x1 commented on pull request #4662: refactor: remove depracted `rand.Seed`
Posted by "d4x1 (via GitHub)" <gi...@apache.org>.
d4x1 commented on PR #4662:
URL: https://github.com/apache/incubator-devlake/pull/4662#issuecomment-1481388994
> Aah, I see. Actually, we don't need the `ENCODE_KEY` to be capitalized. I suggest we just remove the `RandomEncKey` completely and replace it with `RandLetterBytes`
updated.
I was busy with my fulltime job these days, sorry for the late update.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] d4x1 commented on pull request #4662: refactor: remove depracted `rand.Seed`
Posted by "d4x1 (via GitHub)" <gi...@apache.org>.
d4x1 commented on PR #4662:
URL: https://github.com/apache/incubator-devlake/pull/4662#issuecomment-1482740972
> @d4x1 No worries, thank you for your hard work. The code looks good to me. Can you attach some screenshots to demonstrate the new code is working correctly so it can be merged? Thanks for your contribution.
Do you mean add some testing cases? It's a basic refactor, I don't know which screenshots I should provide.
It's just a function and it just works.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] klesh merged pull request #4662: refactor: remove depracted `rand.Seed`
Posted by "klesh (via GitHub)" <gi...@apache.org>.
klesh merged PR #4662:
URL: https://github.com/apache/incubator-devlake/pull/4662
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] d4x1 commented on pull request #4662: refactor: remove depracted `rand.Seed`
Posted by "d4x1 (via GitHub)" <gi...@apache.org>.
d4x1 commented on PR #4662:
URL: https://github.com/apache/incubator-devlake/pull/4662#issuecomment-1469825606
Updated. Fix unnecessary `init`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] klesh commented on pull request #4662: refactor: remove depracted `rand.Seed`
Posted by "klesh (via GitHub)" <gi...@apache.org>.
klesh commented on PR #4662:
URL: https://github.com/apache/incubator-devlake/pull/4662#issuecomment-1474568194
Aah, I see. Actually, we don't need the `ENCODE_KEY` to be capitalized. I suggest we just remove the `RandomEncKey` completely and replace it with `RandLetterBytes`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] d4x1 commented on pull request #4662: refactor: remove depracted `rand.Seed`
Posted by "d4x1 (via GitHub)" <gi...@apache.org>.
d4x1 commented on PR #4662:
URL: https://github.com/apache/incubator-devlake/pull/4662#issuecomment-1471991633
> @d4x1 Yes, I was saying that we should opt for option 2 for now because it is not a true bottleneck of our system. We may opt for 1 in the future after we upgrade to go v1.20 .
Updated. BTW, I change `RandomCapsStr` to non-exportable, it also a function using rand, but it have been used only one place, so there is no need to move it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] klesh commented on pull request #4662: refactor: remove depracted `rand.Seed`
Posted by "klesh (via GitHub)" <gi...@apache.org>.
klesh commented on PR #4662:
URL: https://github.com/apache/incubator-devlake/pull/4662#issuecomment-1482975833
@d4x1 Normally, we would search which functions/features are using the modified code, and verify the related functions/features are working just like before.
In this case, I would set up a brand-new instance to see if the ENCODE_KEY is generated and saved correctly and take screenshots, and so on and so forth.
I can do it if you can not find time, no worries.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] klesh commented on pull request #4662: refactor: remove depracted `rand.Seed`
Posted by "klesh (via GitHub)" <gi...@apache.org>.
klesh commented on PR #4662:
URL: https://github.com/apache/incubator-devlake/pull/4662#issuecomment-1482371220
@d4x1 No worries, thank you for your hard work. The code looks good to me.
Can you attach some screenshots to demonstrate the new code is working correctly so it can be merged?
Thanks for your contribution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org