Skip to content

feat: add function to return a list of asteroids NAIF ids#8

Open
Raselgeuse wants to merge 5 commits into
mainfrom
asteroid-feature
Open

feat: add function to return a list of asteroids NAIF ids#8
Raselgeuse wants to merge 5 commits into
mainfrom
asteroid-feature

Conversation

@Raselgeuse

@Raselgeuse Raselgeuse commented Jun 12, 2026

Copy link
Copy Markdown
Collaborator

Added a function to return an integer list of all asteroid NAIF ids known to Moira.

Summary by Sourcery

Add a public API to retrieve all known asteroid NAIF IDs.

New Features:

  • Expose a new list_asteroid_naifs() helper that returns the integer NAIF IDs of all asteroids known to Moira via ASTEROID_NAIF.

Enhancements:

  • Document the new list_asteroid_naifs() helper alongside existing asteroid query functions and re-export it through the facade module.

@getfoyer

getfoyer Bot commented Jun 12, 2026

Copy link
Copy Markdown

Foyer can't review this PR — paid seat required.

The following commit authors are not members of your Foyer org:

Invite them from https://app.getfoyer.dev/orgs/bbg0NjjDPnEnTbOtj1dS_, or ask an admin to invite them. Foyer will pick the PR up automatically on the next push.

[Foyer · seat-required]

@netlify

netlify Bot commented Jun 12, 2026

Copy link
Copy Markdown

Deploy Preview for heroic-custard-7ea345 ready!

Name Link
🔨 Latest commit f0c39a7
🔍 Latest deploy log https://app.netlify.com/projects/heroic-custard-7ea345/deploys/6a2c55fee1d6620008c00729
😎 Deploy Preview https://deploy-preview-8--heroic-custard-7ea345.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@sourcery-ai

sourcery-ai Bot commented Jun 12, 2026

Copy link
Copy Markdown
Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Adds a new helper function to retrieve all known asteroid NAIF IDs and exposes it through the public facade, mirroring the existing list_asteroids helper for names.

File-Level Changes

Change Details Files
Introduce list_asteroid_naifs() helper to return all asteroid NAIF IDs.
  • Document new list_asteroid_naifs helper alongside existing asteroid utilities in the module header docstring.
  • Implement list_asteroid_naifs() to return the values of the ASTEROID_NAIF mapping as a list of integers, analogous to list_asteroids().
moira/asteroids.py
Expose list_asteroid_naifs() via the public facade API.
  • Add list_asteroid_naifs to the all export list so it is available as part of the public moira API.
moira/facade.py

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@qodo-code-review

qodo-code-review Bot commented Jun 12, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0)

Grey Divider


Informational

1. Top-level spacing inconsistent 🐞 Bug ⚙ Maintainability
Description
The newly added list_asteroid_naifs() is not separated from adjacent top-level functions by two
blank lines, which reduces readability and can conflict with common Python formatting conventions.
Code

moira/asteroids.py[R924-928]

+def list_asteroid_naifs() -> list[int]:
+    """Return the list of asteroid NAIF ids known to Moira (ASTEROID_NAIF values)."""
+    return list(ASTEROID_NAIF.values())

def available_in_kernel(kernel_path: str | Path | None = None) -> list[str]:
Evidence
In the introspection section, def list_asteroids() ends and def list_asteroid_naifs()
immediately follows without the typical two-blank-line separation between top-level function
definitions.

moira/asteroids.py[920-929]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`list_asteroid_naifs()` is placed between two top-level functions without the conventional two blank lines separating top-level `def`s.

### Issue Context
This is a minor formatting/style inconsistency within `moira/asteroids.py`’s public introspection section.

### Fix Focus Areas
- moira/asteroids.py[920-929]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Previous review results

Review updated until commit f0c39a7

Results up to commit f0c39a7


🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0)


Informational
1. Top-level spacing inconsistent 🐞 Bug ⚙ Maintainability
Description
The newly added list_asteroid_naifs() is not separated from adjacent top-level functions by two
blank lines, which reduces readability and can conflict with common Python formatting conventions.
Code

moira/asteroids.py[R924-928]

+def list_asteroid_naifs() -> list[int]:
+    """Return the list of asteroid NAIF ids known to Moira (ASTEROID_NAIF values)."""
+    return list(ASTEROID_NAIF.values())

def available_in_kernel(kernel_path: str | Path | None = None) -> list[str]:
Evidence
In the introspection section, def list_asteroids() ends and def list_asteroid_naifs()
immediately follows without the typical two-blank-line separation between top-level function
definitions.

moira/asteroids.py[920-929]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`list_asteroid_naifs()` is placed between two top-level functions without the conventional two blank lines separating top-level `def`s.

### Issue Context
This is a minor formatting/style inconsistency within `moira/asteroids.py`’s public introspection section.

### Fix Focus Areas
- moira/asteroids.py[920-929]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Qodo Logo

@cloudflare-workers-and-pages

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
❌ Deployment failed
View logs
moira f0c39a7 Jun 12 2026, 06:54 PM

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

@surmado-code-review

surmado-code-review Bot commented Jun 12, 2026

Copy link
Copy Markdown

Automated Checks (advisory, non-blocking)

✅ All checks passed.


Summary

This PR adds a new public helper, list_asteroid_naifs(), in moira/asteroids.py and re-exports it through moira/facade.py so callers can fetch all known asteroid NAIF IDs directly. The implementation is simple and consistent with the existing asteroid helpers; the main review judgment is whether the returned list’s contract is intentional and sufficiently locked down for public use.
Reviewer: most of the risk is in the public API contract of list_asteroid_naifs() in moira/asteroids.py / moira/facade.py — the rest is a straightforward export/docs update.

What to pay attention to

  • moira/asteroids.pylist_asteroid_naifs() returns list(ASTEROID_NAIF.values()) directly. That makes the observable behavior depend on the underlying mapping: if ordering or duplicate values ever matter, those semantics become part of the API whether intended or not.
  • moira/facade.py — because this is re-exported through the facade, any ambiguity here is now part of the package’s public surface rather than an internal convenience helper.

Things I noticed

🟡 Yellow flags — consider for this PR or a follow-up:

  • moira/asteroids.py / moira/facade.py — the main open point from the prior review still applies: there are no tests in this diff for the new public helper or its facade export. Since this expands external API surface, a small test would help prevent accidental contract drift around contents/order/exposure.

Good patterns

  • The helper was added consistently in both the implementation module and the public facade export, which avoids a half-exposed API.
  • The module-level docs were updated alongside the code, so the advertised asteroid helpers stay in sync with the implementation.

Suggested improvements

  1. Add a targeted test for list_asteroid_naifs() and a simple facade-level test that it is exposed where consumers expect it.
  2. Clarify in the docstring whether callers should treat the result as ordered and whether uniqueness is guaranteed, or whether it is intentionally just a direct projection of ASTEROID_NAIF.values().

Questions for the author

  • Should consumers rely on the current ordering of list_asteroid_naifs(), or should they treat the result as unordered?
  • Is it a deliberate API choice that this returns raw mapping values even if multiple asteroid names could theoretically share a NAIF ID?

💡 No engineering standards found. Surmado Code Review works best with a .github/STANDARDS.md that defines your team's conventions. Visit app.surmado.com to create one with the standards assistant — it takes about 5 minutes.


Surmado Code Review (v1.2-mt) is an automated review, designed to work alongside human judgment.

Want to change your STANDARDS.md or YML? Edit it directly, or tune it with our AI agent Scout.

Comment /rerun-review on this PR to refresh the review — costs 1 additional PR credit.

@qodo-code-review

qodo-code-review Bot commented Jun 12, 2026

Copy link
Copy Markdown

PR Summary by Qodo

Expose helper to list asteroid NAIF IDs
✨ Enhancement 📝 Documentation 🕐 10-20 Minutes

Grey Divider

Walkthroughs

Description
• Add list_asteroid_naifs() to return all known asteroid NAIF IDs.
• Document the new helper in moira.asteroids module docs.
• Re-export list_asteroid_naifs() via moira.facade for public API access.
Diagram
graph TD
  U["Library consumer"] --> F["moira.facade"] --> A["moira.asteroids"] --> M[("ASTEROID_NAIF map")]
  A --> N["list_asteroid_naifs()"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Return a sorted list for determinism
  • ➕ Stable output across runs/Python versions and dict construction order changes
  • ➕ More predictable for tests and downstream consumers
  • ➖ Adds implicit ordering semantics (might be unexpected if current order is meaningful)
  • ➖ Minor extra cost to sort (negligible here)
2. Return an immutable tuple instead of list
  • ➕ Prevents accidental mutation of the returned collection
  • ➕ Signals the result is reference data, not meant to be edited
  • ➖ Slightly less ergonomic for callers expecting list operations
  • ➖ Would be a breaking change if callers expect list specifically
3. Expose a single source accessor (e.g., asteroid_naif_map())
  • ➕ Makes it explicit that the mapping is the canonical source of truth
  • ➕ Allows future changes (lazy loading, filtering) behind a stable function
  • ➖ More API surface than needed for the stated requirement
  • ➖ Callers may still need to transform to list themselves

Recommendation: The PR’s approach (a small helper returning ASTEROID_NAIF values, re-exported in the facade) is appropriate and consistent with existing list_asteroids(). If consumer stability matters, consider sorting the returned IDs (or documenting that order is implementation-defined).

Grey Divider

File Changes

Enhancement (2)
asteroids.py Add list_asteroid_naifs() and document it in module overview +5/-0

Add list_asteroid_naifs() and document it in module overview

• Updates the module’s boundary/public-surface documentation to include list_asteroid_naifs(). Adds list_asteroid_naifs() to return ASTEROID_NAIF values as list[int].

moira/asteroids.py


facade.py Re-export list_asteroid_naifs() through the facade +2/-2

Re-export list_asteroid_naifs() through the facade

• Imports list_asteroid_naifs from moira.asteroids and adds it to __all__ so it’s available via the facade public API.

moira/facade.py


Grey Divider

Qodo Logo

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hey - I've left some high level feedback:

  • Consider returning a sorted list of NAIF IDs from list_asteroid_naifs() to guarantee deterministic ordering regardless of how ASTEROID_NAIF is constructed.
  • For naming consistency and clarity, you might want to align the function name with the wording in the docstring, e.g., list_asteroid_naif_ids(), which more clearly conveys that integer IDs are returned rather than strings.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- Consider returning a sorted list of NAIF IDs from list_asteroid_naifs() to guarantee deterministic ordering regardless of how ASTEROID_NAIF is constructed.
- For naming consistency and clarity, you might want to align the function name with the wording in the docstring, e.g., list_asteroid_naif_ids(), which more clearly conveys that integer IDs are returned rather than strings.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

@qodo-code-review

qodo-code-review Bot commented Jun 14, 2026

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit f0c39a7

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