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

New web translator: https://www.e-periodica.ch #3008

Merged
merged 41 commits into from
Sep 21, 2023
Merged

Conversation

aborel
Copy link
Contributor

@aborel aborel commented Mar 28, 2023

Dear all,
this is my attempt at a web translator for the Swiss platform of online journal E-periodica (see https://www.e-periodica.ch/digbib/about1?lang=en ) for more info. This was requested on the Zotero forums https://forums.zotero.org/discussion/comment/426485#Comment_426485 , it seems to work and I hope it meets the expectations:

  • retrieve single reference from an article page (provided sufficient metadata is available, which is not always the case for older digitized journals);
  • retrieve PDF (free access);
  • retrieve multiple refs from a search result page.

Feedback is welcome.

@aborel
Copy link
Contributor Author

aborel commented Apr 20, 2023

Can anybody take a look, and tell me if further actions are needed?

Copy link
Collaborator

@adam3smith adam3smith left a comment

Choose a reason for hiding this comment

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

Some questions, some changes requested -- let me know if you have any questions

E-periodica Switzerland.js Outdated Show resolved Hide resolved
E-periodica Switzerland.js Outdated Show resolved Hide resolved
E-periodica Switzerland.js Outdated Show resolved Hide resolved
E-periodica Switzerland.js Outdated Show resolved Hide resolved
E-periodica Switzerland.js Outdated Show resolved Hide resolved
E-periodica Switzerland.js Outdated Show resolved Hide resolved
E-periodica Switzerland.js Outdated Show resolved Hide resolved
E-periodica Switzerland.js Outdated Show resolved Hide resolved
E-periodica Switzerland.js Outdated Show resolved Hide resolved
E-periodica Switzerland.js Outdated Show resolved Hide resolved
@aborel
Copy link
Contributor Author

aborel commented Apr 27, 2023

There are 2 change requests left that still need so work from me, so perhaps it isn't so bad that the lint check on my latest commit has failed.
@adam3smith can you comment on my questions regarding spacing around colons?

@adam3smith
Copy link
Collaborator

For the Lint -- it's failing because there have been changes to the CI on the main branch and that confuses the process. If you are able to run
git rebase upstream master locally, that'd very likely fix it.

@aborel
Copy link
Contributor Author

aborel commented Apr 27, 2023

OK @adam3smith I think I've got everything in order now.

@aborel
Copy link
Contributor Author

aborel commented May 2, 2023

Do I need to do anything else?

@aborel
Copy link
Contributor Author

aborel commented May 13, 2023

@adam3smith Sorry to bother you, but I don't know if I need to do something at this point, or just be patient until an admin takes care of this pull request (among the hundred other ones that are currently open, so some delay would be completely understandable).

@adam3smith
Copy link
Collaborator

@aborel -- sorry for the late reply. Nothing else for you to do; just had a busy May and Zotero doesn't have someone working on translators full time right now.

Copy link
Collaborator

@adam3smith adam3smith left a comment

Choose a reason for hiding this comment

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

Sorry for the delay here -- a couple more smaller change requests, then this should be gtg

E-periodica Switzerland.js Outdated Show resolved Hide resolved
E-periodica Switzerland.js Outdated Show resolved Hide resolved
E-periodica Switzerland.js Outdated Show resolved Hide resolved
E-periodica Switzerland.js Outdated Show resolved Hide resolved
E-periodica Switzerland.js Outdated Show resolved Hide resolved
E-periodica Switzerland.js Outdated Show resolved Hide resolved
@alex-ter
Copy link
Contributor

alex-ter commented Jul 1, 2023

[...] just had a busy May and Zotero doesn't have someone working on translators full time right now.

@adam3smith, in your opinion, is there anything that could be done by the community to help it? Some things like allowing more people to merge certainly require a very different level of trust, so I'm not talking about that. But maybe there's something like additional testing or reviews, or some such, that would help the maintainers go through the queue quicker, and could be done as some isolated contributions by folks other than maintainers and long-time contributors? I, for one, would be interested.

@adam3smith
Copy link
Collaborator

@alex-ter -- I'm probably the wrong person to ask. I don't work for Zotero, I've just done this for many, many years and they know who I am, so I have commit rights here (which I use fairly conservatively, since I'm not a developer -- I just know a bit of javascript and a lot about metadata), and I think any solution really does have to come from Zotero.

They recognize that they need someone working on this quasi full-time, both to improve existing translators and to manage one of the most low-threshold means of community conributions -- that's why they originally hired AbeJellinek, who then (as I understand it) turned out to be so capable that they moved him to more complex tasks. Not sure if they've hire someone else yet, but that was definitely the plan, and it's going to be hard to make real progress working through PRs without that.

That said, I don't know what permission levels GH allows, but from my perspective, giving people who have committed 2-3 good translators, have some other FLOSS experience, and are either identifiable or identify themselves to the dev team e.g. the ability to approve CI runs would be good. I also think encouraging community reviews of submitted translators (again, by people with some track record) makes sense. (Again, though, I think that'd need to come from Zotero)

The one thing that's challenging about this is that the trickiest part about translators is often not the pure code (though obviously that needs to be tight), but getting metadata import right for Zotero's standards, that requires a deep understanding of how metadata is used and otherwise imported -- see e.g. this recent discussion for an example.

This issue is probably not the right place for this discussion, though. Maybe zotero-dev would be -- not entirely sure on that, either.

@alex-ter
Copy link
Contributor

alex-ter commented Jul 2, 2023

Thank you, that's exactly the insight I was looking for to understand the situation better. This is indeed not the right place for any further discussion (I think zotero-dev is) and I only chimed in here because your comment I quoted was quite aligned with the questions I had after looking at the queue :) Thanks again!

@retorquere
Copy link
Contributor

that said, I don't know what permission levels GH allows

You can get pretty fancy with github actions.

@zoe-translates
Copy link
Collaborator

Hello @aborel, I hope you're still working on this. Thank you for your patience.

I added a few comments based on my understanding. Please free to discuss.

@aborel
Copy link
Contributor Author

aborel commented Aug 14, 2023

@zoe-translates Thanks for the detailed feedback! I'm still very much on board with this, I will study your comments in the next few days.

@aborel
Copy link
Contributor Author

aborel commented Aug 18, 2023

@zoe-translates Your feedback is integrated. Thanks again for your time!

Copy link
Collaborator

@zoe-translates zoe-translates left a comment

Choose a reason for hiding this comment

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

Thanks! There are just two suggestions (see inline annotation), and I think it's ready after that.

E-periodica Switzerland.js Outdated Show resolved Hide resolved
E-periodica Switzerland.js Outdated Show resolved Hide resolved
aborel and others added 3 commits August 18, 2023 08:50
Co-authored-by: zoe-translates <zoe.ma@pm.me>
Co-authored-by: zoe-translates <zoe.ma@pm.me>
@aborel
Copy link
Contributor Author

aborel commented Aug 18, 2023

Suggestions were gladly accepted.

@zoe-translates
Copy link
Collaborator

LGTM!

@aborel
Copy link
Contributor Author

aborel commented Aug 18, 2023

Thank you so much @adam3smith and @zoe-translates , I have learnt as lot!

@zoe-translates
Copy link
Collaborator

Thank you so much @adam3smith and @zoe-translates , I have learnt as lot!

You never know how much I learn from all the contributions :)

@aborel
Copy link
Contributor Author

aborel commented Sep 21, 2023

@adam3smith Does anybody need to do anything to clear the "Changes requested" status for this PR?

@dstillman dstillman merged commit 8e5c648 into zotero:master Sep 21, 2023
1 check passed
@dstillman
Copy link
Member

Sorry for the delay. Merged — thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants