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

Combination dir #1197

Merged
merged 23 commits into from
Sep 30, 2022
Merged

Combination dir #1197

merged 23 commits into from
Sep 30, 2022

Conversation

oleg68
Copy link
Contributor

@oleg68 oleg68 commented Sep 15, 2022

I'm going to start working with combination-related issues: #708, #1195, #1196

This PR moves the combinations-related source files to the separate src/grandorgue/combinations dir.
I've tried to make separate directories for MVC, but unfortunally the current interclass calls do not fullfil the MVC approach.
It is just moving and renaming. No behavior should be changed.

@oleg68 oleg68 added this to the 3.9.0 milestone Sep 15, 2022
@oleg68
Copy link
Contributor Author

oleg68 commented Sep 28, 2022

@rousseldenis could you approve this PR,

Copy link
Contributor

@rousseldenis rousseldenis left a comment

Choose a reason for hiding this comment

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

@oleg68 Thanks for this.

As a recurrent criticism occurs on some forums about code documentation, when such refactoring occurs, shouldn't we include code comments as much as possible ?

This in order to enhance reviewers understanding and further contributors.

@oleg68 oleg68 merged commit 7983348 into GrandOrgue:master Sep 30, 2022
@oleg68
Copy link
Contributor Author

oleg68 commented Sep 30, 2022

@rousseldenis you are right. Most of existing code is not documented now and I don't understand it. When I make some changes in a piece of code, I must to understand it and I'm trying to put some comments.

But the changes like this are more formal: some classes and variables are renamed and their files are moved, and it introduces a lot of formal changes to all references to the renamed/moved objects. Most of the changes are made automatically, So I cann't comment them now.

I doesn't consider commenting the existing code as a completable task, but it is a good practice to add comment to a code piece changed manually.

@oleg68 oleg68 deleted the feature/combinations-dir branch September 30, 2022 08:21
@rousseldenis
Copy link
Contributor

Yes, just a gentle reminder to help people contribute more easily.

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.

3 participants