eshaham / israeli-bank-scrapers

Provide scrapers for all major Israeli banks and credit card companies
MIT License
592 stars 161 forks source link

Roadmap #458

Closed sagiba closed 4 years ago

sagiba commented 4 years ago

Continuing here the discussion from #418:

@esakal wrote:

I mentioned several times that we are considering a way to make this library extendible and extremely powerful. You could have asked for details about it and showed interest; maybe it will do what you want and let you do more powerful things. We have put that on hold till we finalize the Transactipt PR.

Actually I did ask about it back in the very beginning of the thread on #418:

Can you tell more about this new infrastructure? I'm working on some extensions of my own, would be useful if we could sync.

I'd still love to hear more. Is there a consensus that the typescript branch is going to be merged? And that the adapters concept is going to be integrated? Are there any other major changes that are in the works or are planned?

Perhaps it would be useful to have a roadmap document that lists them. Obviously what I am suggesting with extracting the converter logic and adding unit tests for it, requires some major re-factoring (even if it can be done gradually while keeping full backward-compatibility), and it's better to put it in hold until the other big changes are merged into master.

esakal commented 4 years ago

@sagiba Hi, Due to the nature of this library, @eshaham and I try to introduce changes carefully, do everything gradually while maintaining full backward compatibility. We try to avoid having a new major version that breaks the public API and prefers to extend the API so developers will have more control over the flow.

In #384, we experimented with a concept we temporarily call 'adapters'. The goal was to break the flow into logical units, allowing developers to customize the scraping flow, scrape new types of information, or replace only part of the existing logic. The last action described here (replacing part of the existing logic) will probably address your original request in #418, but as you will see, it is only part of the new capabilities

Before continuing reading, I strongly recommend you to read the description of #384. Note that this is a throwaway PR that was created as a playground to feel the impact of such a change.

The outcome of that PR was successful. We managed to break the companies scrapers into logical units (login, data scraping, data projecting) and then do powerful things like using the same login unit while downloading check images, or scraping a list of payments. As a side effect, we extended the public API significantly since we let developers interfere in multiple ways while today the library supports single flow. As we learned that we must adopt a tool that guards and helps developers who are using this library, we created a second POC of adapters using Typescript here, and the outcome was very satisfying.

What guides us is that any change we introduce must not break the current API, and we should be able to gradually refactor the library - we plan to start in reverse order by first adjusting the code to use Typescript #404. We almost completed the Typescript PR; you can check the status there.

After we merge the Typescript PR, we will convert the first company to use the adapters infrastructure (probably Leumi or Leumi card). As I wrote previously, the Adapter PR was an experimental one. We will use it as a baseline for the future adapters design.

Once we will have a company that is based on adapters, we will continue to convert the remaining companies, and we will provide proper documentation.

I recommend you to review the Typescript PR and the Adapters PR. Keep in mind that the adapters PR will not be merged as-is and should be reviewed as a concept PR.

@eshaham feel free to add your insights

esakal commented 4 years ago

@sagiba Hi, Just wanted to hear what from you about the above roadmap. I will soon create an issue asking contributors to review this issue and the Typescript issue and share their insights

sagiba commented 4 years ago

@esakal sounds great, I am eagerly waiting for the TypeScript version to be released (I've been using the TS branch in my internal project for some time now).

re: adapters, I understand the principle, and it's good to have a more flexible framework to allow scraping additional types of data. But this doesn't directly address my request for being able to extract and replace the transaction conversion logic. If I look at: https://github.com/eshaham/israeli-bank-scrapers/blob/adapters-ts/src/visa-cal/adapters/scrape-transactions.js this definitely looks cleaner, but it still couples both mechanical scraping aspects, and logical conversion aspects, which I'd rather see in separate classes.

Maybe let's wait until the TypeScript version is out, and then discuss how to make the adapters even more extendable?

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.