Skip to content

Feature/coding style #1579

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

Merged
merged 8 commits into from
Apr 16, 2025
Merged

Feature/coding style #1579

merged 8 commits into from
Apr 16, 2025

Conversation

ikelos
Copy link
Member

@ikelos ikelos commented Jan 27, 2025

As part of #1578 I figured we should really get the coding style more front and center. This is where we can work together on a branch to get it in shape and make sure everyone's happy. We should include reasoning behind our decisions to avoid arguing or bikeshedding. Even if it ends up in here, it doesn't mean anything's set in stone, just that that's how we do it for now, but if there's a good reason to change in the future, then we can revise it...

If you all could take a look at it and put in anything we tend to do that I've forgotten to note down, or things you don't agree with (or that we don't do or stuff you think we should do), at least it'll be recorded and then we can discuss it to reach a consensus. 5:)

ikelos and others added 3 commits March 25, 2025 17:25
Thanks, my fingers don't work as well as they used to, so I appreciate you picking up typos like this!

Co-authored-by: Donghyun Kim <[email protected]>
Co-authored-by: Donghyun Kim <[email protected]>
Hehehe, good catch, thanks!  5:D

Co-authored-by: Donghyun Kim <[email protected]>
@eve-mem
Copy link
Contributor

eve-mem commented Mar 28, 2025

I'm not sure if my comments are coding style...

What about suggestions for variables names for common things? e.g. a task for linux processes or context for the context. It's used almost everywhere in the framework like this - to not do so would be confusing to me.

What about the order of variables for a function, e.g. context first, then layer/symbol names, then other bits. It's almost always in that kind of order I think? What about suggesting passing things like layers by name rather than the actual objects. You touch on these in the classmethods part, but maybe suggest it for everything?

@dgmcdona
Copy link
Contributor

I don't know if this is the best place for this or not, but this scenario has cropped up a couple of times now and it's worth paying attention to during dev + code review. We'll often use lists/sets/dicts to store offsets that we've already seen, but what ends up getting added is not a simple primitive but an objects.Pointer or objects.Integer which are substantially larger than the Python primitive, and it ends up blowing up memory usage. Maybe worth a mention in the style guide?

@ikelos
Copy link
Member Author

ikelos commented Apr 11, 2025

Sure, feel free to add commits to the branch adding appropriate sections? It's not supposed to be an edict from on high, it's supposed to be a collaborative effort we all add to to get right... 5:)

@ikelos
Copy link
Member Author

ikelos commented Apr 16, 2025

Doesn't feel like anyone's had massive issues with this, so I'm going to merge it in and then people can PR changes they may want...

@ikelos ikelos merged commit fa73298 into develop Apr 16, 2025
25 checks passed
@ikelos ikelos deleted the feature/coding-style branch April 16, 2025 09: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.

4 participants