Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(internet)!: ip now returns ipv4 and ipv6 #1059

Merged
merged 8 commits into from Oct 14, 2022
Merged

feat(internet)!: ip now returns ipv4 and ipv6 #1059

merged 8 commits into from Oct 14, 2022

Conversation

xDivisionByZerox
Copy link
Member

@xDivisionByZerox xDivisionByZerox commented Jun 11, 2022 •
edited

Is listed in #1044.

This is a breaking change and should not be merged before the next major release.

@xDivisionByZerox xDivisionByZerox added c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs breaking change Cannot be merged when next version is not a major release labels Jun 11, 2022
@xDivisionByZerox xDivisionByZerox added this to the vFuture milestone Jun 11, 2022
@xDivisionByZerox xDivisionByZerox requested a review from a team as a code owner June 11, 2022 12:36
@xDivisionByZerox xDivisionByZerox self-assigned this Jun 11, 2022
@xDivisionByZerox xDivisionByZerox requested a review from a team June 11, 2022 12:36
@codecov
Copy link

codecov bot commented Jun 11, 2022 •
edited

Codecov Report

Merging #1059 (6718cfd) into next (9c1437d) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             next    #1059      +/-   ##
==========================================
+ Coverage   99.60%   99.62%   +0.01%     
==========================================
  Files        2164     2164              
  Lines      236816   236816              
  Branches      999     1004       +5     
==========================================
+ Hits       235888   235917      +29     
+ Misses        906      877      -29     
  Partials       22       22
Impacted Files Coverage Δ
src/modules/internet/index.ts 100.00% <100.00%> (ø)
src/modules/internet/user-agent.ts 89.41% <0.00%> (+7.67%) ⬆️

ST-DDT
ST-DDT reviewed Jun 11, 2022
View reviewed changes
test/internet.spec.ts Outdated Show resolved Hide resolved
@Shinigami92 Shinigami92 changed the title refactor(internet.ip): return v4 or v6 refactor(internet.ip)!: return v4 or v6 Jun 11, 2022
@Shinigami92
Copy link
Member

I also updated the access to any definition.internet with an optional chaining operator since my IDE was complaining that the property is potentially undefined.

@xDivisionByZerox I would prefer that you extract this into its separate PR
But I also never had an issue with that 🤔 Also TypeScript check didn't complain yet 🤔
So is it maybe just your IDE? Did you miss to switch e.g. to the installed TypeScript instead of the embedded IDE TS version? Sometimes that can lead to such issues.

@xDivisionByZerox
Copy link
Member Author

I also updated the access to any definition.internet with an optional chaining operator since my IDE was complaining that the property is potentially undefined.

@xDivisionByZerox I would prefer that you extract this into its separate PR But I also never had an issue with that 🤔 Also TypeScript check didn't complain yet 🤔 So is it maybe just your IDE? Did you miss to switch e.g. to the installed TypeScript instead of the embedded IDE TS version? Sometimes that can lead to such issues.

I have no problems providing an additional PR regarding this topic, so we can focus on the logic changes in this PR.

@xDivisionByZerox
Copy link
Member Author

xDivisionByZerox commented Jun 11, 2022 •
edited

I did revert the optional chaining. @Shinigami92 @ST-DDT please review again.

ST-DDT
ST-DDT previously approved these changes Jun 11, 2022
View reviewed changes
@ST-DDT
Copy link
Member

ST-DDT commented Jun 11, 2022

I have no problems providing an additional PR regarding this topic, so we can focus on the logic changes in this PR.

IMO there is no need to change it.

  • There is no lint warning
  • It doesnt make the code more readable or safe.
  • Its only for tests

@ST-DDT ST-DDT requested a review from a team June 11, 2022 16:20
@xDivisionByZerox xDivisionByZerox added do NOT merge yet Do not merge this PR into the target branch yet p: 1-normal Nothing urgent labels Jun 11, 2022
@xDivisionByZerox xDivisionByZerox mentioned this pull request Jun 11, 2022
Closed
@xDivisionByZerox xDivisionByZerox added the s: accepted Accepted feature / Confirmed bug label Jun 13, 2022
ST-DDT
ST-DDT reviewed Jul 2, 2022
View reviewed changes
test/internet.spec.ts Outdated Show resolved Hide resolved
test/internet.spec.ts Outdated Show resolved Hide resolved
ST-DDT
ST-DDT previously approved these changes Jul 3, 2022
View reviewed changes
import-brain
import-brain previously approved these changes Jul 3, 2022
View reviewed changes
@xDivisionByZerox xDivisionByZerox added m: internet Something is referring to the internet module needs rebase There is a merge conflict and removed s: accepted Accepted feature / Confirmed bug labels Jul 28, 2022
import-brain
import-brain previously approved these changes Aug 18, 2022
View reviewed changes
@ST-DDT ST-DDT added needs rebase There is a merge conflict do NOT merge yet Do not merge this PR into the target branch yet and removed do NOT merge yet Do not merge this PR into the target branch yet labels Oct 12, 2022
@xDivisionByZerox xDivisionByZerox removed the needs rebase There is a merge conflict label Oct 13, 2022
@ST-DDT ST-DDT requested review from a team October 13, 2022 16:38
@ST-DDT ST-DDT added the s: accepted Accepted feature / Confirmed bug label Oct 13, 2022
@Shinigami92 Shinigami92 changed the title refactor(internet)!: return v4 or v6 ip feat(internet)!: ip now returns ipv4 and ipv6 Oct 14, 2022
@Shinigami92 Shinigami92 enabled auto-merge (squash) October 14, 2022 21:49
@Shinigami92 Shinigami92 merged commit a90f2fe into next Oct 14, 2022
@ST-DDT ST-DDT deleted the refactor/internet/ip branch November 8, 2022 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ST-DDT ST-DDT ST-DDT approved these changes

@Shinigami92 Shinigami92 Shinigami92 approved these changes

@import-brain import-brain import-brain approved these changes

Assignees

@xDivisionByZerox xDivisionByZerox

Labels
breaking change Cannot be merged when next version is not a major release c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs m: internet Something is referring to the internet module p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Faker Roadmap
Status: Done
Milestone
v8.0 - Module Re-Shuffling
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@xDivisionByZerox @Shinigami92 @ST-DDT @import-brain