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

Deprecate helpers.createTransaction() #782

Closed
ST-DDT opened this issue Apr 5, 2022 · 7 comments
Closed

Deprecate helpers.createTransaction() #782

ST-DDT opened this issue Apr 5, 2022 · 7 comments
Labels
c: chore PR that doesn't affect the runtime behavior good first issue Good for newcomers has workaround Workaround provided or linked p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug

Comments

@ST-DDT
Copy link
Member

ST-DDT commented Apr 5, 2022

Clear and concise description of the problem

The method provides a range of methods that are loosely related to each other.
Most users will usually require more specific data models or other property names.

Suggested solution

The method should be deprecated for removal similar to the card methods.

Alternative

Upgrade the method to create transactions for the current/recent/future date.

Additional context

No response

@ST-DDT ST-DDT added c: chore PR that doesn't affect the runtime behavior p: 1-normal Nothing urgent s: needs decision Needs team/maintainer decision labels Apr 5, 2022
@ST-DDT ST-DDT added this to the v6 - Non-Breaking Changes milestone Apr 5, 2022
@ST-DDT ST-DDT moved this to Todo in Faker Roadmap Apr 5, 2022
@Shinigami92
Copy link
Member

We can safely remove the s: needs decision because this was just forgotten to be marked as deprecated.
I assume I just searched for card* stuff.

@Shinigami92 Shinigami92 added s: accepted Accepted feature / Confirmed bug and removed s: needs decision Needs team/maintainer decision labels Apr 5, 2022
@ST-DDT ST-DDT added the good first issue Good for newcomers label Apr 5, 2022
@Shinigami92
Copy link
Member

closed via #916
I removed it intentionally because I do not think that anyone uses it
Even if, they should create their own function

Repository owner moved this from Todo to Done in Faker Roadmap May 4, 2022
@robross0606
Copy link

We we use it. LOL. It would be nice if there were deprecation notes with a recommended alternative approach.

@Shinigami92
Copy link
Member

We we use it. LOL. It would be nice if there were deprecation notes with a recommended alternative approach.

Thanks 🙏
That's valuable feedback. Can you provide some more context so we can see how you used it.
We once decided to not provide "complex" objects as result anymore so faker-consumers just build there own functions wrapping faker.
Can we help you somehow with this?

@ST-DDT
Copy link
Member Author

ST-DDT commented May 31, 2024

It would be nice if there were deprecation notes with a recommended alternative approach.

We have improved our upgrading guideline rules in the meantime.
Could you please check whether the new migration guide would satisfy your requirements?

https://next.fakerjs.dev/guide/upgrading.html


We removed these methods because the requirements for the complex return objects differ between most users and thus an individualistic approach with these methods usually results in better usability. We are sorry for the inconvenience that was caused by this change.

Here is a workaround, that you can use as an alternative to the built-in method:

function createTransaction(): {
  amount: string;
  date: Date;
  business: string;
  name: string;
  type: string;
  account: string;
} {
  return {
    amount: faker.finance.amount(),
    date: faker.date.recent(),
    business: faker.company.name(),
    name: `${faker.finance.accountName()} (...${faker.string.numeric(4)})`,
    type: faker.helpers.arrayElement(
      faker.definitions.finance.transaction_type
    ),
    account: faker.finance.accountNumber(),
  };
}

Tested with v8.4.1:

{
  amount: '428.32',
  date: 2024-05-30T20:17:49.443Z,
  business: 'Hodkiewicz, Gottlieb and Effertz',
  name: 'Savings Account (...7362)',
  type: 'withdrawal',
  account: '96287636'
}

@ST-DDT ST-DDT added the has workaround Workaround provided or linked label May 31, 2024
@robross0606
Copy link

robross0606 commented May 31, 2024

The missing thought process here is that someone upgrading the library with pre-existing usage needs to go hunt down how to satisfy previous behavior to not break existing behavior. That means understanding what was being done in the built-in method and then mimicking it. I was able to do this, but it required digging into the (now removed) code to see how it was done. The migration notes basically don't account for this use case at all, amounting to "You're on your own. Do what you need.". What we needed is to reproduce what was being done in the old version so as to not break pre-existing behavior.

@matthewmayer
Copy link
Contributor

I think this has something which has improved over time. The 6 to 7 migration guide was quite terse. 7 to 8 and the upcoming 8 to 9 migration guides are hopefully better about providing explicit alternatives for removed functionality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: chore PR that doesn't affect the runtime behavior good first issue Good for newcomers has workaround Workaround provided or linked p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Status: Done
Development

No branches or pull requests

4 participants