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

Repository Refactoring (proposal) #82

Open
8 of 10 tasks
erikyo opened this issue Apr 30, 2024 · 15 comments · May be fixed by #86
Open
8 of 10 tasks

Repository Refactoring (proposal) #82

erikyo opened this issue Apr 30, 2024 · 15 comments · May be fixed by #86

Comments

@erikyo
Copy link
Collaborator

erikyo commented Apr 30, 2024

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
Copy link
Owner

smhg commented May 2, 2024

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
Copy link
Collaborator Author

erikyo commented May 2, 2024

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
Copy link
Owner

smhg commented May 3, 2024

@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
Copy link
Collaborator Author

erikyo commented May 3, 2024

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
Copy link

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

@erikyo
Copy link
Collaborator Author

erikyo commented May 3, 2024

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.

@erikyo erikyo linked a pull request May 4, 2024 that will close this issue
@erikyo erikyo linked a pull request May 5, 2024 that will close this issue
@johnhooks
Copy link

@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 #86 (comment)

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
Copy link
Collaborator Author

erikyo commented May 8, 2024

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
Copy link
Collaborator Author

erikyo commented May 13, 2024

@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
Copy link
Owner

smhg commented May 13, 2024

@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
Copy link
Collaborator Author

erikyo commented May 13, 2024

The most important changes were made here:

#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
Copy link
Owner

smhg commented May 14, 2024

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
Copy link
Collaborator Author

erikyo commented May 14, 2024

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
Copy link
Owner

smhg commented May 14, 2024

@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
Copy link
Collaborator Author

erikyo commented May 14, 2024

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)

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 a pull request may close this issue.

3 participants