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.

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

refactor(helpers): deprecate unique method #1790

Merged
merged 21 commits into from Apr 1, 2023

Conversation

Shinigami92
Copy link
Member

@Shinigami92 Shinigami92 commented Jan 29, 2023

For now this is just a preparation for

and marks the method helpers.unique as a soft-deprecation
This means for now that we wont throw a warning at runtime but just want to indicate in IDEs like VSCode that it is not safe anymore to rely on this function as it will not be maintained by us anymore and we want to remove it in the long run.

This PR should not auto-close #1785

@Shinigami92 Shinigami92 added c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs m: helpers Something is referring to the helpers module deprecation A deprecation was made in the PR labels Jan 29, 2023
@Shinigami92 Shinigami92 self-assigned this Jan 29, 2023
@Shinigami92 Shinigami92 requested a review from a team as a code owner January 29, 2023 13:05
@codecov
Copy link

codecov bot commented Jan 29, 2023

Codecov Report

Merging #1790 (68e6f97) into next (f5eddaa) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 68e6f97 differs from pull request most recent head 0e02a2a. Consider uploading reports for the commit 0e02a2a to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             next    #1790   +/-   ##
=======================================
  Coverage   99.62%   99.62%           
=======================================
  Files        2464     2460    -4     
  Lines      240210   240160   -50     
  Branches     1278     1281    +3     
=======================================
- Hits       239313   239268   -45     
+ Misses        874      869    -5     
  Partials       23       23           
Impacted Files Coverage Δ
src/modules/helpers/index.ts 99.05% <100.00%> (+<0.01%) ⬆️

... and 10 files with indirect coverage changes

import-brain
import-brain previously approved these changes Jan 29, 2023
@ST-DDT
Copy link
Member

ST-DDT commented Jan 29, 2023

This means for now that we wont throw a warning at runtime

Could you please explain the reason why you did it like this instead of our usual deprecation workflow?

@Shinigami92
Copy link
Member Author

This means for now that we wont throw a warning at runtime

Could you please explain the reason why you did it like this instead of our usual deprecation workflow?

Yes, it is because I searched for alternatives, but didn't found one yet.
I'm still thinking about to publish a package on my own, but not so sure if I want to do this as I would love to have it more as a community driven open source package. This could still be possible but not sure if my GitHub account is this much known right now.

Secondly I want to deprecate it like this right now so no further maintain request would come in like e.g. https://github.com/faker-js/faker/issues/456 (linked like that so I do not link the issues)

So, until we do not have an alternative we could point to, I wont like to annoy our userbase.

@Shinigami92
Copy link
Member Author

So, until we do not have an alternative we could point to, I wont like to annoy our userbase.

Having a deprecation warning in the codebase could also be considered anoying IMHO.

It will not warn, but just strikes through
it would only warn if you have configured an eslint rule

@ST-DDT
Copy link
Member

ST-DDT commented Jan 29, 2023

IMO we should only consider removing this if there is already replacement for it somewhere.

@Shinigami92
Copy link
Member Author

IMO we should only consider removing this if there is already replacement for it somewhere.

Please suggest a different approach to show users that this function is decided in last team meeting on 2023-01-26 that we as faker maintainer will not maintain this function anymore in the future

@ST-DDT
Copy link
Member

ST-DDT commented Jan 29, 2023

Other than having an open issue?

I only agree to this if there is an issue blocking v8.0 to document a replacement and the removal of the exclusion customization added by this PR.
I can write the issue for that if you would like me to.

@Shinigami92
Copy link
Member Author

Other than having an open issue?

I only agree to this if there is an issue blocking v8.0 to document a replacement and the removal of the exclusion customization added by this PR. I can write the issue for that if you would like me to.

No, this is against what we decided here: #1785 (comment)
The replacement is not blocking v8.0 🤷

@ST-DDT
Copy link
Member

ST-DDT commented Jan 29, 2023

I only agree to this if there is an issue blocking v8.0 to document a replacement and the removal of the exclusion customization added by this PR. I can write the issue for that if you would like me to.

No, this is against what we decided here: #1785 (comment) The replacement is not blocking v8.0 🤷

Yes, currently it wouldn't be a blocking issue, but if you want to merge this PR in v8.0, then it would become one.
IMO: You could directly point to this comment as a replacement and proceed with a normal deprecation.

@Shinigami92
Copy link
Member Author

I only agree to this if there is an issue blocking v8.0 to document a replacement and the removal of the exclusion customization added by this PR. I can write the issue for that if you would like me to.

No, this is against what we decided here: #1785 (comment) The replacement is not blocking v8.0 shrug

Yes, currently it wouldn't be a blocking issue, but if you want to merge this PR in v8.0, then it would become one. IMO: You could directly point to this comment as a replacement and proceed with a normal deprecation.

ok 👌 I will do so

@Shinigami92 Shinigami92 changed the title refactor(helpers): mark unique as soft deprecation refactor(helpers): mark unique as deprecation Jan 29, 2023
@Shinigami92 Shinigami92 changed the title refactor(helpers): mark unique as deprecation refactor(helpers): deprecation unique method Jan 29, 2023
@ST-DDT ST-DDT changed the title refactor(helpers): deprecation unique method refactor(helpers): deprecate unique method Jan 29, 2023
ST-DDT
ST-DDT previously approved these changes Jan 29, 2023
Copy link
Member

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

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

While I personally don't like this deprecation.
This is what the team has decided on.
The change itself looks good to me.

@ST-DDT ST-DDT added the p: 1-normal Nothing urgent label Feb 4, 2023
Co-authored-by: Eric Cheng <ericcheng9316@gmail.com>
@ST-DDT ST-DDT requested review from a team March 23, 2023 17:31
Copy link
Member

@xDivisionByZerox xDivisionByZerox left a comment

Choose a reason for hiding this comment

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

I want to publicly +1 @ST-DDT's statement.
I don't like deprecating this in the knowledge that there is no real alternative to this.
Nevertheless, this was decided in a team meeting and I respect the decisions (and arguments) made there.

@Shinigami92
Copy link
Member Author

For future read, please have a look first into #1785 before just visiting and complaining about this PR

@Shinigami92 Shinigami92 enabled auto-merge (squash) April 1, 2023 09:47
@Shinigami92 Shinigami92 merged commit dbbc785 into next Apr 1, 2023
15 checks passed
@ST-DDT ST-DDT deleted the mark-unique-as-for-deprecation branch April 1, 2023 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs deprecation A deprecation was made in the PR m: helpers Something is referring to the helpers module p: 1-normal Nothing urgent
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants