-
Notifications
You must be signed in to change notification settings - Fork 66
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
Autocompletion of configure.py
#357
Conversation
…fork into autocomplete
Conflicts: doc/wiki/Installation:-Configure.py.md
Conflicts: doc/wiki/configure.py.md src/configure.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the work on this PR.
After I try to use it, I found that it only support --arg option
.
Is it possible to support --arg=option
?
If compgen
does not support it. Can we achieve this by modifing the top level option when = is detected after a valid arg?
@technic960183 Thanks for the comment and review. I have updated the code accordingly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update, --arg=option
works great. And I don't have any idea of a better implementation now.
Since our wiki use python configure.py
as examples, I think support python configure.py
is necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid autocomplete being triggered on unrelated scripts named configure.py
, I suggest verifying whether the configure.py being executed is the intended one.
I propose two possible solutions:
- Use an Identifier for Validation
Add or utilize an identifier to confirm the script. For example:
If ./configure.py -h
outputs 2023 Computational Astrophysics Lab, NTU. All rights reserved.
at the end, you can validate it by checking for Computational Astrophysics Lab
.
Alternatively, introduce a new option such as --version
and verify if the output matches a predefined pattern.
- Verify Without Executing the Script
This approach avoids callingconfigure.py
altogether, which can be important if the other script has side effects (e.g., writing logs) when executed.
Use the sha1sum
of configure.py
, for instance:
026c866b02f873dad60290a43336ee555119a834 configure.py
Hard-code this hash into the autocomplete script and compare it. However, note that the hash would need to be updated every time configure.py
is modified.
The above resolutions are suggestions. The choice of implementation is up to you and @hyschive to decide based on the project's priorities and constraints.
@technic960183 Thanks for the detailed comments. The My current strategy to verify the script is:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution.
There are some bugs of side effects outside the GAMER directory related to falling back to the default behavior of auto-completition.
Checking the very first commit hash to verify the file is a brilliant idea!
@technic960183 I can not reproduce the bugs you found. I guess these bugs are due to the different version behavior. May I know your My environment:
|
@ChunYen-Chen
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the bug fix.
I left some comments for improving efficiency and UX.
Autocompletion: Add trailing space back
Conflicts: doc/wiki/Installation.md src/configure.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found some possible edge cases and some consideration on the beginners.
Feel free to reject these suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests done:
- The installation process on Wiki.
- Correct behavior of the autocompletion.
- Does not affect other
python
,python3
command. - Does not affect other
./configure.py
(inathena
for example) - Build the Wiki for a preview.
Although I approved this PR, please notice that a bug can be triggered if there is an alias of python
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ChunYen-Chen This is super useful! I've left some inline comments. Additionally, please address the following points before merging:
- Do not declare empty symbolic constants like
-DGPU_COMPUTE_CAPABILITY=
in theMakefile
when disabling the corresponding options (e.g.,--gpu
). - File an issue report for this bug
- Several option choices have changed due to the newly added
prefix
feature (e.g.,--dual, --elbdm_scheme, --wave_scheme, --gramfe_scheme, --hybrid_scheme
). Please update both the Wiki page andgenerate_make.sh
accordingly.
@technic960183 Thanks for the careful review!
Co-authored-by: Hsi-Yu Schive <[email protected]>
It is hard to memorize all the option names since
configure.py
has too many options. Now, you do not need to memorize the options anymore!By simply pressing tab key, the available options will be shown and be autocompleted if there is only one available option.
Other updates
-lh
shows the help message in alphabetical order.Makefile.log
will not be generated with-h
,-lh
, and--autocomplete_info
arguments.--verbose_make
parser.add_argument()