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

Enabled Reviewer Credits Plugin for OJS 3.4 and added Reviewer Finder… #281

Merged
merged 4 commits into from
Jul 9, 2024

Conversation

ReviewerCredits
Copy link

Enabled Reviewer Credits Plugin for OJS 3.4 and added Reviewer Finder Feature. Fixed some bugs and added error messages.

@ReviewerCredits
Copy link
Author

Hi @asmecher , I couldn't understand what is the issue with the CI. Can you help me out please? Thanks.

@asmecher
Copy link
Member

@ReviewerCredits, this appears to be a glitch in the Travis CI system, not related to your PR. I'll see if it goes away in a few hours.

@asmecher
Copy link
Member

@ReviewerCredits, rather than trying to maintain a single codebase that's compatible with several releases of OJS, I'd suggest creating branches for each line of releases that match the ones we have in our repos (stable-3_3_0, stable-3_4_0) and having that repo only be compatible with the line of releases. It'll simplify the code a lot, and make it much less likely that you'll introduce a regression. You can fully modernize the code (e.g. adding namespaces, changing the .inc.php suffix to .php, etc) in the newest branch, and that'll be a lot easier to evolve going forward to 3.5.

This isn't a requirement for merging into the plugin gallery as-is, but in addition to cleaning up your code a lot it'll also make it a lot easier to code-review :)

While it might seem like more work to maintain several branches, trust me, it's a lot easier this way in the end! (We've done this with all of our plugins.)

@ReviewerCredits
Copy link
Author

Hi again @asmecher , Thanks for your comments. So we will take your suggestion and create 2 separate branches for OJS 3.3 and OJS 3.4. So it is easier to maintain. While we re-work it we would appreciate it if you have any other comments/reviews on the code already. So we can handle them simultaneously. Thanks. Emre.

@ReviewerCredits
Copy link
Author

Dear @asmecher the code is changed and updated to reflect your suggestion. Hope it is good to merge like this. Thanks.

@ReviewerCredits
Copy link
Author

Hi again @asmecher , did you have time to check this PR out? Feel free to let us know if we should fix anything. Thanks.

@asmecher
Copy link
Member

asmecher commented Jul 4, 2024

@ReviewerCredits, sorry for the wait! Here are some recommendations:

  • Rename your branches to match what we use: stable-3_3_0 for OJS 3.3.0, and stable-3_4_0 for 3.4.0.
  • For the 3.4.0 compatible branch:
    • Rename .inc.php files to .php and move your classes into namespaces. (See "Additional Details for Plugins" section of this comment.) After this, require / import calls should no longer be needed.
    • Remove classes.php -- it looks like it's no longer used.
    • Remove conditional tests around class existence (example, but there are others) -- this is probably left over from the 3.3.0 compatibility code.
    • Likewise, there's still some compatibility code left in ReviewerCredits::getTemplatePath that should be removed.
    • Use getters/setters rather than accessing the _data attribute on DataObject subclasses directly -- this will become protected/private in the future

And corrected the recommendations.
@ReviewerCredits
Copy link
Author

@asmecher Thank you very much for your recommendations. We've just completed them. It should be ready to merge.

@asmecher asmecher merged commit 1d31a03 into pkp:main Jul 9, 2024
1 check passed
@asmecher
Copy link
Member

asmecher commented Jul 9, 2024

Thanks, @ReviewerCredits, I've merged this!

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