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

Fix paths and regenerate files after recent changes #127

Merged
merged 3 commits into from
Jan 26, 2024

Conversation

trey0
Copy link
Contributor

@trey0 trey0 commented Jan 26, 2024

Should be an easy review. Just fixing breakage from other recent PRs, no substantive changes.

@trey0 trey0 requested review from Bckempa and bcoltin January 26, 2024 16:33
@bcoltin
Copy link
Member

bcoltin commented Jan 26, 2024

The changes to the modules are going to conflict with this one: #125
which I think will be better in the long run? (moves python files to src, executables just call the src python code)

@trey0
Copy link
Contributor Author

trey0 commented Jan 26, 2024

The changes to the modules are going to conflict with this one: #125 which I think will be better in the long run? (moves python files to src, executables just call the src python code)

Sorry, I must have missed the notification email about your PR and wish I had looked it over first. Without reviewing in detail yet, I suspect that your PR doesn't address all the issues this one does, but maybe gets some of them?

Also, this would be the third time somebody is rearranging the directory structure since I started writing these scripts, and every time it broke the scripts and I had to fix them. If you are moving things around again, please make sure you can run the three Python scripts without any arguments and they do something sane.

@bcoltin
Copy link
Member

bcoltin commented Jan 26, 2024

Yes, I haven't touched the yaml files or PDDL files.

@trey0
Copy link
Contributor Author

trey0 commented Jan 26, 2024

Started over from scratch, making similar updates starting from the version of develop after Brian's PR was merged.

- name: Run isort
run: |
isort . --diff --extend-skip cmake --profile black --check-only
isort . --diff --extend-skip=cmake --extend-skip=submodules --extend-skip=astrobee/survey_manager/survey_dependencies --profile=black --check-only
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Skipping folders of external code we shouldn't lint.)

"--extend-skip=astrobee/survey_manager/survey_dependencies"
"--profile=black"
)
if $(isort . "${isort_args[@]}" --diff --check-only --quiet >/dev/null); then
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's important to use . here as explained in the comment above and not restrict isort to looking at changed files. The --extend-skip stuff is to avoid linting external code.

Note this is following how it's done in the astrobee repo, except in my local testing I found that you need to use multiple --extend-skip arguments instead of a single arg with a comma-separated list.

Copy link
Member

@bcoltin bcoltin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you

@trey0 trey0 merged commit 7d7852b into nasa:develop Jan 26, 2024
4 checks passed
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