Feature/faster build #5417
Feature/faster build #5417
Conversation
|
Seems a bit faster on my local builds....even without the --fast option |
khancyr
force-pushed
the
feature/faster-build
branch
2 times, most recently
from
1bf5630 to
4bb5cee
Compare
There was a problem hiding this comment.
I think I might quite like the meat of these changes.
But they are mixed in with a whole heap of changes to update the Python code. Most of which are good - unicode string stuff, utf-declarations, fstrings etc. This just makes it far more difficult to review than it should be.
This is only a partial review as I got frustrated with working through changes unrelated to the changes in the parameter and log information download.
scripts/cap_params.py
Outdated
| @@ -14,18 +14,18 @@ | |||
|
|
|||
| for f in args.files: | |||
| print("Processing %s" % f) | |||
| txt = open(f, 'r').read() | |||
| txt = open(f).read() | |||
There was a problem hiding this comment.
I think PathLib is the more-modern way still...
scripts/cap_params.py
Outdated
| matches = re.findall(r'[,.\s][A-Z][A-Z0-9]+_[A-Z_]+[,.\s]', txt) | ||
| matches = re.findall(r'[,.\s][A-Z]+_[A-Z_]+[,.\s]', txt) | ||
| changed = False | ||
| for m in matches: | ||
| s = str(m) | ||
| param = s.strip() | ||
| s2 = "%s:ref:`%s<%s>`%s" % (s[0], param, param, s[-1]) | ||
| s2 = f"{s[0]}:ref:`{param}<{param}>`{s[-1]}" |
There was a problem hiding this comment.
Was this change tested?
| subprocess.check_call(["powershell.exe", "Start-BitsTransfer", "-Source", fetchurl]) | ||
| else: | ||
| subprocess.check_call(["wget", fetchurl]) | ||
| def fetch_and_rename(fetchurl: str, target_file: str, new_name: str) -> None: |
There was a problem hiding this comment.
This yieldssyntax isn't in all of Python3, is it? When did it come in?
There was a problem hiding this comment.
this comes with python3.5 and it is really usefull to track bugs !
| f'{dataname.lower()}.rst', site, cache) | ||
|
|
||
|
|
||
| def fetchlogmessages(site: Optional[str] = None, cache: Optional[str] = None) -> None: |
There was a problem hiding this comment.
These types make things rather less readable.
|
@peterbarker I didn't intend to make a huge python3 style update, but Charlie said : hey why not use f-string. So I ran pyupgrade to modernize all .py in one go. Beside the file downloading, I didn't intend to modify the logic nor refactor everything. |
You could probably do that style update and rebase on top of that, then. I'm still a touch worried about (a) the "yields" syntax which I thought was relatively new, and the readability with the Types system. OTOH, I was tired and a bit grumpy on first review, so we'll see what happens on a second review :-) Love f-strings :-) |
|
Sphinx 5.1.1 is the current minimum for the ArduPilot wiki. That requires python 3.6 or newer. Generators showed up before then so everything is good there. Type hints are also great for quickly coming up to speed with what is happening. They don't do much until coupled with a linter or static analyzer. |
Ah, sorry, I was referring to what's apparently known as "return type annotations". I was using the older meaning of "yields" as synonymous with "returns", as opposed to the Pythonesque "yields" for generators.
Are they, 'though? It's harder to just glance at a function definition and see what it's taking. I do think we should find out - but introducing their syntax really should be done in a separate PR. If we can show that flake8 can now pick up more bugs, bringing it in would be a no-brainer IMO. I don't think one could argue that it does make the syntax more scary, 'though. Please note that you don't need my approval to merge this PR. If you want to merge it with Henry's blessing that's workable. It's just that if it breaks its likely to stay broken until you fix it :-) We're not running out of PR numbers. I would have merged the unicode and f-string changes instantly. |
|
@peterbarker flake8 doesn't do type checking. The best tool for now is mypy or IDE if it support typing. |
|
@peterbarker Peter, still needs changes? or can I merge? |
|
@khancyr @peterbarker says his review stands....I wont merge unless you two come to an understanding...sorry |
|
no problem, I will improve this |
khancyr
force-pushed
the
feature/faster-build
branch
2 times, most recently
from
0bccf34 to
9165821
Compare
|
@peterbarker removed py3 upgrade change, just the speedup now, will make an other PR for py3 |
khancyr
force-pushed
the
feature/faster-build
branch
from
9165821 to
79bc00f
Compare
correct some python code on update.py
convert wget call to pure python equivalent that are faster. Make the download files from https://autotest.ardupilot.org in parallele to speed up the build