Skip to content

Infrastructure: rename pF -> pParentWidget & fix some whitespace layouts#6379

Merged
Kebap merged 5 commits intodevelopmentfrom
Kebap-patch-2
Nov 14, 2022
Merged

Infrastructure: rename pF -> pParentWidget & fix some whitespace layouts#6379
Kebap merged 5 commits intodevelopmentfrom
Kebap-patch-2

Conversation

@Kebap
Copy link
Copy Markdown
Contributor

@Kebap Kebap commented Oct 20, 2022

Brief overview of PR changes/additions

make variable name more readable

Motivation for adding to Mudlet

Improve readability

Other info (issues closed, discussion etc)

Fix #6375

@Kebap Kebap requested a review from a team as a code owner October 20, 2022 14:54
@Kebap Kebap requested a review from a team October 20, 2022 14:54
@add-deployment-links
Copy link
Copy Markdown

add-deployment-links bot commented Oct 20, 2022

Hey there! Thanks for helping Mudlet improve. 🌟

Test versions

You can directly test the changes here:

No need to install anything - just unzip and run.
Let us know if it works well, and if it doesn't, please give details.

@mudlet-machine-account mudlet-machine-account added this to the 4.17.0 milestone Oct 20, 2022
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Oct 20, 2022

Warnings
⚠️ PR makes changes to 14 source files. Double check the scope hasn't gotten out of hand

Generated by 🚫 dangerJS against 95eacdd

Copy link
Copy Markdown
Member

@SlySven SlySven left a comment

Choose a reason for hiding this comment

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

👍 LGTM but can we simultaneously also move those initialisation list entries to each be on a separate line - we used to do that but I think the clang-format doesn't do that automagically and we now have a mix of styles. What is nice about the one-per-line is that it is clearer if something gets added/removed/reordered - and whilst I am sure someone could object to this on the basis that it should be done in a separate PR it seems silly to defer that to that when it only affects constructors and we are already hitting one chunk of them here (though it is pushing at the dangerous limits 😀 ).

@Kebap
Copy link
Copy Markdown
Contributor Author

Kebap commented Oct 21, 2022

I am sure someone could object to this on the basis that it should be done in a separate PR

That would be me, no?

it only affects constructors and we are already hitting one chunk of them here

Ok let's assume I changed all of these here...

and we now have a mix of styles.

We still have a mix afterwards.

Funny thing is, this tiny issue scope and PR stem from exactly the same kind of commenting related issues at PRs. It then got buried for almost 2 years after that PR was merged, until I accidentally stumbled across it while researching something else.

There seems to be some sort of reluctance to post an actual issue to keep track of an idea, and a separate PR to fix the issue in the whole code base, which would actually be required to heal the mix of styles, yes?

Co-authored-by: Stephen Lyons <slysven@virginmedia.com>
@Kebap Kebap changed the title Infrastructure: rename pF -> pParentWidget Infrastructure: rename pF -> pParentWidget & fix some whitespace layouts Oct 21, 2022
@Kebap Kebap enabled auto-merge (squash) October 21, 2022 17:27
@SlySven
Copy link
Copy Markdown
Member

SlySven commented Oct 21, 2022

Funny thing is, this tiny issue scope and PR stem from exactly the same kind of commenting related issues at PRs. It then got
buried for almost 2 years after that PR was merged, until I accidentally stumbled across it while researching something else.

Okay - I'll try to run something up over this weekend to hit the remaining constructors - if I don't you can call me out on Discord...! 😏

@Kebap
Copy link
Copy Markdown
Contributor Author

Kebap commented Oct 21, 2022

No it's not important enough to do over the next 2-3 days immediately. Just open a low priority issue to remind yourself and others

SlySven added a commit to SlySven/Mudlet that referenced this pull request Oct 23, 2022
This is so they all have each member initialiser on a single line - so that
changes can be more easily seen/comprehended in Git differences (as opposed
to them all being on a long single line...!)

This is a response to the comment:
Mudlet#6379 (comment)

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
SlySven added a commit to SlySven/Mudlet that referenced this pull request Oct 23, 2022
This is so they all have each member initialiser on a single line - so that
changes can be more easily seen/comprehended in Git differences (as opposed
to them all being on a long single line...!)

This is a response to the comment:
Mudlet#6379 (comment)

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
SlySven added a commit that referenced this pull request Oct 24, 2022
…6387)

This is so they all have each member initialiser on a single line - so
that changes can be more easily seen/comprehended in Git differences (as
opposed to them all being on a long single line...!)

This is a response to the comment:
#6379 (comment)

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
SlySven added a commit that referenced this pull request Oct 24, 2022
…6388)

This is so they all have each member initialiser on a single line - so
that changes can be more easily seen/comprehended in Git differences (as
opposed to them all being on a long single line...!)

This is a response to the comment:
#6379 (comment)

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
@Kebap Kebap merged commit e832e70 into development Nov 14, 2022
@Kebap Kebap deleted the Kebap-patch-2 branch November 14, 2022 09:14
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.

make pF variable more readable

4 participants