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
base: master
Are you sure you want to change the base?
Elibrary quick fix #3289
Conversation
There was a problem hiding this 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.
There was a problem hiding this 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.
@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. |
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. |
"url": "https://www.elibrary.ru/item.asp?id=22208210", | ||
"url": "https://elibrary.ru/item.asp?id=22208210", |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Elibrary moved datablock to div[2]. That is changed.
Test changes reverted as unnecessary.
EmbeddedMetadata.js is untouched, Cyberleninka separate translator will be implemented.