PresearchOfficial / presearch-packages

Instant information packages for the Presearch engine
123 stars 49 forks source link

crypto/fiat conversion search terms #136

Closed joshtom1 closed 2 years ago

joshtom1 commented 2 years ago

Describe the bug The crypto/fiat conversion search can be incorrect.

To Reproduce image

Expected behavior The search should be more precise.

Screenshots See above

Desktop (please complete the following information):

Smartphone (please complete the following information): N/A

Additional context I built a currency converter in Nov, but it appears not to be used. I think the search was a little more precise. If you found something wrong, I would have been happy to fix but I didn't receive much feedback. I guess I'm curious why my package was not used.

I was out for a month because of covid, but other than that I have been following the project. I sent an email and created a pull request. This is the only response I got, but I assumed everything was okay: image

I wanted to contribute because I was passionate about the project and wanted to help. It was not for fame or economic incentives but I must admit it is a little weird to see the same package built by a different developer, later on, and with less features.

I was just searching for cpus and not trying to purposefully find an issue with the new converter.

Here is part of the email I wrote to the dev team that shows some of the features: image

Here are the results with the current converter: image

image

image

The search term doesn't always trigger: image image

joshtom1 commented 2 years ago

The nzd to sol conversion seems consistent today. Maybe it was an issue with the api the other day.

jejopl commented 2 years ago

Hey @joshtom1, first of all - I'm really sorry that I've missed your email - my bad 🙏.

About this situation, since both packages could be triggered with the same query, we have decided to go with currencyConverter package, since it has 'all in one' functionality. We wanted to avoid the situation where query can trigger more than one package. I agree that yours is more efficient, for cryptocurrency conversions. I've improved the test environment and now, you will receive a warning message, when the query will trigger a different package, than that you are working on (it will be pushed to github soon, need to do some final tweaks)

End of the year was pretty busy for us, we had a lot of things going on and there wasn't enough time to cover the packages.

Recently, we have changed the approach and now, we want to focus more on the packages. There's a new discord channel, dedicated to presearch-packages and it would be cool if you could join us as well - https://discord.gg/ErehmHWk

Decision to temporarily remove your package was made, since it was quicker to do it than refactor both of the packages. We wanted to keep consistency between the layout, so the package should look very similar for crypto and fiat conversions. Now I can see that this decision was bad and not fair. I want to apologize you, since I'm responsible for the packages and it was mostly my personal call. In the future, we will want to have a place, where the community could vote for their favorite package

We will keep both of the packages, I'll remove the crypto functionality from currencyConverter package, and the queries for crypto conversion, should now trigger only cryptoConverter package. Or we can merge those two packages and have you as co-author

It would be cool if you could change the styling of your package and make it look similar to currencyConverter package. Let me know if you are willing to do this change.

jejopl commented 2 years ago

Yeah, I've talked with the team and we decided that we will combine those two packages into one. We will use trigger function from cryptoConverter and the rest from currencyConverter

joshtom1 commented 2 years ago

Thanks for the reply. I got a little excited about contributing on github (I come from a different dev background). I shouldn't expect my code to be used. But I do think it would be good to communicate with a dev and give them the opportunity to adjust things. It is possible I missed a communication but I try to follow the project closely.

I also know that there was an older converter and I didn't want to step on any toes. Thats why I gave the heads up that there was some overlap. It might make sense to combine fiat and crypto converters like you are doing. I will have to think about it some more. I don't think it was a bad decision to scrap my converter and I did join the discord. I was just really surprised by the outcome.

For the issue, I think Niklas is doing a good job and adjusted the regex. It is hard for a regex or other strategy to be 100%. I think as long as its 99% accurate, it will result in a nice UX. It does have a nice way to search for conversions, such as 'nzd sol'. I am aware that is a common way to search but my package doesn't have that yet. So that is one advantage for the new one.

I think as the project evolves, it will get more advanced. Especially when Presearch starts to index websites and can get smarter about what a user wants. In the meantime, I thought it would be nice to be a little smart about it. For example, searching for nicknames/descriptions, in addition to currency codes.

I know sometimes it can be better to have less code because of maintenance and what not. But I thought the UX of flexible searching was a stronger priority.

Thank for you for updating the test environment to check for overlapping triggers. That will be important, the more packages there are.

I can adjust the styling. I thought that might come back to me :). I'm not a designer but as a user I didn't really prefer the existing styling. But if flexible search is desired and we decide to combine fiat/crypto into 1 converter, then I may just see what it will take to combine the packages (which would use the existing styling).

jejopl commented 2 years ago

Hey @joshtom1, closing this one since we merged those two packages into one, and added you as co-author

joshtom1 commented 2 years ago

Sounds good.

I've been thinking about this for a while. It should be 1 package because it should be able to handle fiat to crypto conversions.

Also, I was thinking of holding off on working on this for 2 reasons. 1 - I don't want to interfere with Niklas ability to quickly add features, etc... 2 - I am in the job market and I want to avoid a potential conflict of interest.

I don't need to be made a co-author :). Feel free to use whatever features you like in my old code.

I look forward to using the new conversion package on presearch. There is also potential to have conversions for various other common finanical assets like commodities/metals. I look forward to seeing how this evolves.