Skip to content

add yandex browser#108

Merged
aylen384 merged 1 commit intoopenSUSE:masterfrom
13werwolf13:add_yandex-browser
Aug 8, 2022
Merged

add yandex browser#108
aylen384 merged 1 commit intoopenSUSE:masterfrom
13werwolf13:add_yandex-browser

Conversation

@13werwolf13
Copy link
Contributor

due to the fact that yandex-browser is finally out of beta.

@aylen384
Copy link
Member

aylen384 commented Aug 5, 2022

You have conflicting queries, so this won't work.
Also it would make more sense to put this into a single file (and class) and ask the user, which version they want to install.
Especially as the repo is the same for beta and stable.
See https://github.com/openSUSE/opi/blob/master/opi/plugins/ms_edge.py for an example how this can be done.

@13werwolf13
Copy link
Contributor Author

You have conflicting queries, so this won't work. Also it would make more sense to put this into a single file (and class) and ask the user, which version they want to install. Especially as the repo is the same for beta and stable. See https://github.com/openSUSE/opi/blob/master/opi/plugins/ms_edge.py for an example how this can be done.

sorry but the repos are NOT the same. However, I understood what to do and will do.
However, I did not understand about the conflict?

@aylen384
Copy link
Member

aylen384 commented Aug 5, 2022

Oh yea - sorry the repo really seems to be different. But is should still work with a single class. I just don't want to clutter the list of plugins.

About the conflict:

queries = ('yandex-browser-stable', 'yandex-browser', 'yandex')
queries = ('yandex-browser-beta', 'yandex-browser', 'yandex')

... must not have entries that exist in any other plugin - how should opi otherwise know, which plugin it should run.
I updated the testsuite to check for this case.

@13werwolf13
Copy link
Contributor Author

... must not have entries that exist in any other plugin - how should opi otherwise know, which plugin it should run. I updated the testsuite to check for this case.

now I understand. well, I thought that he would give a choice as in a normal text search. i was wrong. I will fix it and send a commit today, but a little later. Thanks.

@13werwolf13
Copy link
Contributor Author

@asdil12 I'm not sure if I did everything right, but I hope so.

Copy link
Member

@aylen384 aylen384 left a comment

Choose a reason for hiding this comment

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

See my comments - also please squash your changes into a single commit and test that everything works.

@13werwolf13
Copy link
Contributor Author

13werwolf13 commented Aug 5, 2022

@asdil12 sorry, i not know how do that

also please squash your changes into a single commit

i do

git rebase -i fd4008d7fb53a031a28e25c82b9a023871bcbe84
git push --force

and i get

Everything up-to-date

or should I delete this PR and create a new one from new branch?

@13werwolf13
Copy link
Contributor Author

ah, I figured out how to do it .. fixed.

@13werwolf13 13werwolf13 requested a review from aylen384 August 6, 2022 12:08
@aylen384 aylen384 merged commit e3cd314 into openSUSE:master Aug 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants