Closed TelosTelos closed 3 years ago
Note: of course, don't throw away what you've written. It is much appreciated and you can copy-paste most of it to new, subsequent, more focused PRs.
Commit with Black format: https://github.com/Lonami/pyndustric/commit/5f371bc346ca0087b91522e263925979d173a4f3
Before pushing changes, run the following commands in your terminal, with the current working directory being the root of the project (much like you would use git
; install only needs to be done once):
pip install black
black .
Oops, yeah, I'd meant to have this replace compiler.py -- not sure how I messed that up. Will read through the more detailed comments soon.
I can't speak for other users, but my practice before running some new github project is to read the code, starting with the import lines, to get a sense of what it does and how malicious it might be. My guess is that our average bear won't know what ast
is, and may not have the time and good will to go look it up elsewhere, so would appreciate having a quick comment on the import line explaining what this module is and why the project imports it. I certainly would have appreciated it.
More generally, I try to imagine what sort of person is likely to read my code and write comments that they'd be likely to find useful. This project seems to be aimed at a quite general audience of Mindustry players who are familiar enough with Python that they can tell they'd rather write Mindustry logic in Python than in straight mlog, but may be suspicious about what our project is doing. So, I'd expect that people who go read the top few lines to have a fair bit of general Python familiarity, but not necessarily to be familiar with esoteric python modules like ast or even dataclass. So I think it's best to provide them a bit of overview and assurance that we have good reason to import the things we import.
As for comments further down, this is more likely to be read by serious enthusiasts and potential contributors -- e.g., people like me. So my general rule of thumb is to include whatever comments I myself would have found useful when I'd read it the first time, and that forgetful-me probably will find useful if/when I come back to a project again months or years later.
You asked about ***
. That was just a note to myself that what I wrote there needed more attention. That's a longstanding personal convention of mine, in writing both code and papers. It might be better for me to stick with "TODO" which serves roughly the same role, and is picked up by PyCharm syntactic highlighting.
Many of your suggested rephrasings of comments seem fine. I was writing a lot of those during the process of trying to figure out what the code does, aiming to ease that process for whomever the next reader is. I'm sure there's room for lots of improvement in phrasing, especially by someone like you who knows ast and your code better than I do.
My next steps are likely to be doing quite a few of the TODO's I marked. A lot of them are pretty interdependent, esp. if I'm reworking expression-handling so that complex expressions like 2*x+1
can be used anywhere a simple variable like x
could have been used, and if I'm doing the equivalent of operator-overloading to allow vectorized operations. The other stuff I've been thinking of doing, like an object-oriented interface for block control and unit control, and changing the way that arguments are passed to functions, I'll probably integrate with the reworked expression + vector system, so it'd be hard to separate too.
Given how interdependent many of these changes are, I'm not sure how much sense it makes to try to split these into separate pull requests. Instead, if I get this to work (I currently feel about 80% confident that I will), then you'll probably be faced with the choice of whether you want to switch over to this reworked way of doing things or just leave my version as a separate fork. Either's fine with me.
Probably the most helpful feedback you could give at this point would be thoughts about the various TODO's, e.g., if you think some of them wouldn't work out or would be incompatible with other things you'd like to end up doing (the main such incompatibility I foresee is my replacing the stack with dummy variable usage for speed and simplicity --vs-- your eventually implementing namespaces and full-fledged recursion which really would require a stack), if you think there'd be a better way of going about them, or if you see that I'm mistaken about the places I'd marked as the likely places to handle different sorts of expressions.
My guess is that our average bear won't know what
ast
is, and may not have the time and good will to go look it up elsewhere, so would appreciate having a quick comment on the import line explaining what this module is and why the project imports it.
The best option here is probably to make a CONTRIBUTING.md
guide which briefly describes the project's structure and briefly introduces third party modules used, like ast
, to explain what they do and why they're used. But I don't think it's suited for a comment. Again it's not our job to document third party modules in our code, but we can have a contributing guide which guides new contributors, and this information would fit here.
but may be suspicious about what our project is doing
They can inspect the generated output and review it to make sure it makes sense. Or just trust the project like I generally do. Or setup wireshark to make sure it's not performing network traffic if they're not very code friendly.
But even if you were to inspect the code, probably users wouldn't try to dive too deep into what it's doing, as long as they don't spot anything extremely weird like use of sockets.
It might be better for me to stick with "TODO" which serves roughly the same role, and is picked up by PyCharm syntactic highlighting.
Yes, let's prefer standard practices. I use TODO myself as well.
My next steps are likely to be doing quite a few of the TODO's I marked
I've thought about adding you as a contributor to the project, but I would still prefer if you made changes through pull requests so that I can review them. Not adding you as a contributor forces you to do follow this workflow. But I think I'll just add you and trust you won't make controversial changes without asking for feedback.
Probably the most helpful feedback you could give at this point would be thoughts about the various TODO
A lot of them you're opening as issues though? Which are a better fit for discussion, really, but I can go read over the TODOs here.
This doesn't change any functionality yet, but does add a lot of explanatory comments that should make the code a lot more readable for other contributors, and adds a lot of TODO's mostly corresponding to things we've discussed in issues.