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

Elibrary quick fix #3289

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Elibrary quick fix #3289

wants to merge 2 commits into from

Conversation

advoropaev
Copy link
Contributor

@advoropaev advoropaev commented Mar 27, 2024

Elibrary moved datablock to div[2]. That is changed.
Test changes reverted as unnecessary.
EmbeddedMetadata.js is untouched, Cyberleninka separate translator will be implemented.

@advoropaev advoropaev changed the title Library Elibrary quick fix Mar 27, 2024
Embedded Metadata.js Outdated Show resolved Hide resolved
Copy link
Contributor

@alex-ter alex-ter left a comment

Choose a reason for hiding this comment

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

This PR actually contains two unrelated changes, I'd sugest updating a summary for proper trace. Other comments inline.

Embedded Metadata.js Outdated Show resolved Hide resolved
Embedded Metadata.js Outdated Show resolved Hide resolved
Embedded Metadata.js Outdated Show resolved Hide resolved
eLibrary.ru.js Outdated Show resolved Hide resolved
eLibrary.ru.js Outdated Show resolved Hide resolved
Copy link
Member

@AbeJellinek AbeJellinek left a comment

Choose a reason for hiding this comment

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

Please revert test changes and other issues that were pointed out in inline comments, but otherwise this seems fine.

Embedded Metadata.js Outdated Show resolved Hide resolved
Embedded Metadata.js Outdated Show resolved Hide resolved
Embedded Metadata.js Outdated Show resolved Hide resolved
@alex-ter
Copy link
Contributor

alex-ter commented Apr 6, 2024

@advoropaev, do you plan to rework this one, or something? In case you aren't interested in continuing (which is of course perfectly fine), I can take over and steward both fixes you had, while addressing the review comments. Just let me know.

@advoropaev advoropaev reopened this Apr 6, 2024
@advoropaev
Copy link
Contributor Author

I have reverted unnecessary changes. Cyberleninka has metadata embedded and suits for EM translator, but first/last author name is reverse, so it apparently needs a separate translator.

Comment on lines -739 to +722
"url": "https://www.elibrary.ru/item.asp?id=22208210",
"url": "https://elibrary.ru/item.asp?id=22208210",
Copy link
Contributor

@alex-ter alex-ter Apr 7, 2024

Choose a reason for hiding this comment

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

This is the opposite of all the other test URL changes. When opening those URLs manually, I see that www. part is actually not needed and it's stripped by the web server. In view of that, this particular change seems meaningful, while the opposite ones (adding the www.) are questionable. Why those are needed?

Copy link
Contributor

@alex-ter alex-ter Apr 7, 2024

Choose a reason for hiding this comment

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

edit note: if you read the email notification or the web page, earlier, version of this comment, disregard that, I updated it significantly based on further analysis.

I ran a couple of tests using Scaffold in both Z6 and Z7 (now with your changes applied, unlike the first time I wrote this comment - sorry for the noise), and looks like it's the Z6 peculiarity - it apparently "toggles" that www. part - adds it where it's absent and removes it where it's present, which is weird. Z7 on the contrary, only adds, but does not remove. @AbeJellinek, I wonder whether this needs further analysis, what do you think? Z6 behaviour looks completely incorrect, but I'm not sure Z7's is fully correct either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's right, but from my experience it is more random. For instance you can run It several times and occasionally it'll change. I had only url in diff and not all the time. I tried both Z6 and Z7, maybe you are right about Z7, but Z6 seems to do it randomly and only with this website, haven't seen this happening for other translators. So usually I thought it is the site issue, this toggling, then blocking tests with captcha.

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

3 participants