smhg / gettext-parser

Parse and compile gettext po and mo files, nothing more, nothing less
MIT License
158 stars 43 forks source link

Repository Refactoring (proposal) #82

Open erikyo opened 2 months ago

erikyo commented 2 months ago

In order to avoid making a huge PR with generic changes, I'm going to write a tracking issue here that lists the changes I would like to propose, in that way we will have a sort of changes tracking to isolate changes by 'type'.

This issue supersedes #80, which involved a series of structural changes too delicate to be made all at once.

smhg commented 2 months ago

Thank you! I think you are overdoing it a bit for the smaller changes :) But your effort is great! Let's start with 2) and 3) I guess?. If you keep the PR's focussed on a clear set of changes, I can merge quickly.

erikyo commented 2 months ago

yep that the reason why i decided to breakdown the previous issue into multiple tasks, this way the pr will be much less loaded and consequently quicker to verify

if you kindly create a development branch, I make my pull request target that branch and we can work there without modifying the master (at least until all or part of these modifications are completed).

what do you think? Unfortunately, some modifications, especially those that require files to be moved, would lead to merge conflicts and some tasks (such as task 2) require that previous pr's have already been merged.

ps all these changes have already been made in a fork I just need to cherrypick changes. ps all these changes have already been made in a fork, I just have to choose the changes. Actually I would have more, for example the parsing process could be speeded up by about 20% (tested)

smhg commented 2 months ago

@erikyo would you be ok to join this project as a maintainer? I don't have enough time currently to follow up on your (very much appreciated) efforts. They seem well thought out and it would only make sense you can take care of this yourself if you ask me. Please let me know and I will add you here on GitHub.

erikyo commented 2 months ago

Yes i can't hide that I would be very pleased but at the same time I am aware that this is a very heavily used repository and i will still need a second eye to approve any changes to the master. If it's ok for you, it would be really cool for me. Thanks!

johnhooks commented 2 months ago

@erikyo I would help support you with code review and feedback.

erikyo commented 2 months ago

That would be great! Your experience is certainly a great help 🚀

ATM I'm moving the pull requests to the development branch, so they can be merged without touching the master for the time being.

johnhooks commented 1 month ago

@erikyo I have a little more context now reading this issue.

It seems where the breakdown between this issue and PR #86, plus my comment https://github.com/smhg/gettext-parser/pull/86#issuecomment-2100414408

If the goal is to use the development branch as a staging for larger changes to master, everything that is merged into development has to go through CR and be approved by the maintainer. Otherwise, the PRs for development -> master have too many unrelated changes.

erikyo commented 1 month ago

Consider this branch as a pre-publication quality control checkpoint. I've included you in this process so that you have the "last word" over it.

My intention was to utilize that specific branch within the 'Release Branching Strategy.' However, to avoid inconveniencing the maintainer (who generously provided me with the opportunity), I independently handled the pull request aggregation on this branch. I assigned him the task of overseeing the development pull requests, allowing him to review each one separately and maintain control over the final outcome before merging it with the master branch.

erikyo commented 1 month ago

@smhg we are close to having completed the set of improvements, what do you think? All that is missing at the moment are the tests (to achieve 100% coverage) and to update the readme. Consider that doing the tests is also intrinsically revising the pr and unless problems arise we can merge (without regret) the new changes.

smhg commented 1 month ago

@erikyo can you please tell me what to look at? I've received so many notifications over the last few days I couldn't possibly read them all (I'll look for a way to disable those).

erikyo commented 1 month ago

The most important changes were made here:

https://github.com/smhg/gettext-parser/pull/89

That pr many lines have been changed but on the other hand nothing of the functional code level have been touched as the most of the changes were related to the jsDocs (which at the end is a comment)

smhg commented 1 month ago

I've looked at #89. Its title is 'fix missing types and jsDocs'. Yet it contains a plethora of different changes? It also does not seem to be based on master? So there is more preceding history? Isn't what you're asking me to look at the diff between the master and development branches?

erikyo commented 1 month ago

So there is more preceding history?

Yes, that's correct. The development branch serves as a staging ground for a series of iterative changes aimed at achieving a comprehensive improvement. Each pull request builds upon the previous one, gradually refining the codebase towards the desired outcome. PR #89 represents an intermediate step in this process, addressing specific issues related to missing types and jsDocs. While the changes may appear extensive, they primarily focus on enhancing documentation rather than altering core functionality. It's worth noting that PR #89 encapsulates the most significant and foundational change within the entire set of modifications.

Isn't what you're asking me to look at the diff between the master and development branches?

Absolutely, reviewing the diff between the master and development branches or specifically examining PR #89 provides insight into the incremental modifications made throughout the development process. By doing so, you can track the evolution of the codebase and understand the rationale behind each change. Moreover, focusing on the development branch grants a holistic view of the cumulative improvements, facilitating a comprehensive assessment before merging.

smhg commented 1 month ago

@erikyo let's take a step back. Your approach doesn't work for me currently. I feel I have a responsibility to current en future users to hand over this library in a careful way. This means I need to be involved still, whether that will be relevant to the future direction of this library or not.

I think many of your contributions are valuable for this project. I am actually not thinking about your contributions themselves so much. You seem to be very much interested in growing the functionality of this library and have invested a lot of effort in it. That's great!

It seems though like we haven't been able to find common ground to cooperate yet.

Are you open to discuss a different approach? Or you feel like your efforts can stand on their own and it is better to release your fork? I don't want to waste your time discussing changes you don't consider necessary.

erikyo commented 1 month ago

Could you please elaborate on what specific aspects of my approach aren't aligning with your current needs or expectations? Understanding this would be invaluable in moving forward collaboratively.

Moreover, I'm open to discussing alternative approaches that might better suit our collective goals. If there are particular areas where you believe adjustments or clarifications are needed, I'm eager to address them constructively.

----edit ps. if the problem is the 'development' branch, however, know that it was used for this series of modifications and that I do not think it will be needed any more in the future (although I believe it is good practice)