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

Inline - macros #1541

Closed
Closed

Conversation

nyurik
Copy link
Contributor

@nyurik nyurik commented Feb 7, 2024

Per #1517 (comment) consolidate all macros-related PRs

Per pgcentralfoundation#1517 (comment) consolidate all macros-related PRs
@nyurik
Copy link
Contributor Author

nyurik commented Feb 7, 2024

@workingjubilee I was trying to understand the reasoning in #1532 (review) and not sure I got it. This PR simply inlines formatting args - making the format strings simpler to read. This is usually a good step before major refactoring - the simpler the code, the easier it is to massively refactor it and spot issues. Why would this PR preclude any kind of further modification?

@workingjubilee
Copy link
Member

workingjubilee commented Feb 7, 2024

This is usually a good step before major refactoring - the simpler the code, the easier it is to massively refactor it and spot issues. Why would this PR preclude any kind of further modification?

There are a few different reasons. Key amongst them is I am often drawn to solve irritants as they are brought to my attention. The code in pgrx-sql-entity-graph fell into such disrepair and was so awful to engage with that people barely touched it for literally an entire year.

I resolved to address the problem and from there I did make a few PRs, as I mentioned a while ago, that made it just tolerable enough to read and work with. However, I do not wish for it to become something I can "live with", or I risk losing interest in the more significant reworks. I have observed myself doing this before, so I know it is the case that I function this way.

The other reason is that

This PR simply inlines formatting args - making the format strings simpler to read.

I do not necessarily find them easier to read. Code can be made more readable by making it more concise, but it is in a different way than how the code is made more readable by it being more decomposed. You have already observed my small complaints about my ability to read things, and how it can get worse when things are clustered together.

Yes, I probably should go visit an ophthalmologist, but the problem is probably purely neurological.

@nyurik
Copy link
Contributor Author

nyurik commented Feb 7, 2024

heh, fair enough. I guess we all have some neurological issues - I for example tend to first focus on minor things that jump out at me, such as non-consise code, or minor nits. In the process of fixing them, I begin to see bigger issues - at least to me, nits tend to obfuscate bigger code problems, so ... just like with my kids, i need to cleanup the room just a bit to start seeing where things should go. Again, I will probably not be able to spend as much time on this code as you would, so you should drive these decisions - whatever style ends up with better code overall is what we will all win from. Thx!

@workingjubilee
Copy link
Member

I do experience that as well, but sometimes leaving such tiny low-hanging fruit helps me get started.

And indeed, I will be happy to let go of my preference once I'm not quite such a deciding factor (or even just when the small fixes are not so intimately related to the code's need for improvements).

Thank you for your work!

@nyurik nyurik deleted the entity-graph-lints branch February 7, 2024 03:19
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