Skip to content

[next] refactor(utils): Replace GenRandomId with getRandomId #6425

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

Merged
merged 2 commits into from
Mar 3, 2025

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Jan 23, 2025

☑️ Resolves

  • Use Typescript for correct type annotations
  • Fix length problem (the old implementation sometimes did not yield the expected length, e.g. if a "short" random number was generated)¹
  • Add unit test for the util

¹ Did some benchmarks: Just the method is now ~10% slower. But the old method delivered too short values for ~25% of all cases.

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable
  • 3️⃣ Backport to next requested with a Vue 3 upgrade

@susnux susnux added 3. to review Waiting for reviews vue 3 Related to the vue 3 migration refactor ♻️ Pull request that is neither a fix nor a feature labels Jan 23, 2025
@susnux susnux added this to the 9.0.0-alpha.7 milestone Jan 23, 2025
@susnux susnux force-pushed the chore/refactor-getrandomid branch from 8c8a1c6 to a64d43a Compare January 23, 2025 12:26
ShGKme

This comment was marked as resolved.

@susnux

This comment was marked as resolved.

@susnux susnux force-pushed the chore/refactor-getrandomid branch from 068c557 to 9d885b8 Compare January 23, 2025 18:11
@susnux

This comment was marked as outdated.

@susnux susnux requested a review from ShGKme January 24, 2025 18:35
@Antreesy

This comment was marked as resolved.

@susnux susnux mentioned this pull request Feb 16, 2025
13 tasks
@susnux susnux force-pushed the chore/refactor-getrandomid branch 2 times, most recently from d52226f to 9f4a107 Compare February 28, 2025 13:50
@susnux
Copy link
Contributor Author

susnux commented Feb 28, 2025

Adjusted @Antreesy should be good now.

@Antreesy
Copy link
Contributor

CI wouldn't agree with you 😆

I changed NcDateTimePickerNative inbetween, and someone else touched NavigationSettings:
image

@ShGKme

This comment was marked as resolved.

@susnux susnux force-pushed the chore/refactor-getrandomid branch from 9f4a107 to c5f6f21 Compare March 2, 2025 21:25
Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine with any name

@susnux susnux force-pushed the chore/refactor-getrandomid branch from 6e2d4eb to 52dae93 Compare March 3, 2025 14:54
susnux added 2 commits March 3, 2025 16:05
- Use Typescript for correct type annotations
- Fix length problem (the old implemenation sometimes did not yield the
  expected length, e.g. if a "short" random number was generated)¹
- Add unit test for the util

¹ Did some benchmarks: Just the method is now ~10% slower. But the old
method delivered too short values for ~25% of all cases.

Signed-off-by: Ferdinand Thiessen <[email protected]>
@susnux susnux force-pushed the chore/refactor-getrandomid branch from f6cdd97 to 38f9254 Compare March 3, 2025 15:05
@susnux susnux modified the milestones: 9.0.0-alpha.7, 9.0.0-alpha.8 Mar 3, 2025
@susnux susnux merged commit 479da20 into next Mar 3, 2025
24 checks passed
@susnux susnux deleted the chore/refactor-getrandomid branch March 3, 2025 15:29
@ShGKme
Copy link
Contributor

ShGKme commented Mar 3, 2025

Backport to master?
It is not a breaking change

@ShGKme ShGKme changed the title refactor(utils): Replace GenRandomId with getRandomId [next] refactor(utils): Replace GenRandomId with getRandomId Mar 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews refactor ♻️ Pull request that is neither a fix nor a feature vue 3 Related to the vue 3 migration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants