feat(v2): Add auto-initializing v2 API with cleaner interface#11
feat(v2): Add auto-initializing v2 API with cleaner interface#11astro-fusion merged 2 commits intomainfrom
Conversation
- Add createSweph() factory that auto-initializes native module - Add SwephInstance with clean method signatures using options objects - Add comprehensive v2.test.ts with 22 tests - Update README with v2 API documentation and examples - Mark legacy API (createSwephAdapter, initializeSweph) as deprecated - Fix @af/sweph-core package.json exports (remove non-existent .mjs) - Enable vitest for @af/sweph-node package BREAKING CHANGE: v2 API is now the recommended interface. Legacy API remains available but is deprecated.
Summary of ChangesHello @astro-fusion, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant enhancement to the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is a great pull request that introduces a much cleaner v2 API, complete with auto-initialization, comprehensive tests, and updated documentation. The new API is a significant improvement for the library's usability. My review focuses on enhancing the consistency and correctness of this new API and its documentation. I've identified a potential bug in timezone handling and a few areas in the README examples that could be clarified to provide a better experience for developers.
| const geoLoc = { | ||
| latitude: location.latitude, | ||
| longitude: location.longitude, | ||
| timezone: location.timezone ?? 0, |
There was a problem hiding this comment.
The timezone from the opts parameter is being ignored; only location.timezone is considered. This is inconsistent with calculatePlanets and can lead to unexpected behavior if a user provides a timezone in the options. The timezone from opts should be prioritized to ensure consistent behavior across the API.
timezone: opts?.timezone ?? location.timezone ?? 0,| async function main() { | ||
| // Create instance (auto-initializes native module) | ||
| const sweph = await createSweph(); | ||
|
|
||
| // 2. Perform calculations | ||
| const date = new Date(); | ||
| const planets = sweph.calculatePlanets(date, { | ||
| ayanamsa: 1, // Lahiri | ||
| location: { latitude: 27.7, longitude: 85.3 } | ||
| // Calculate planetary positions | ||
| const planets = await sweph.calculatePlanets(new Date(), { | ||
| ayanamsa: AYANAMSA.LAHIRI, | ||
| timezone: 5.75, // Nepal | ||
| }); | ||
|
|
||
| console.log(planets); | ||
| console.log('Sun:', planets.find(p => p.id === 'sun')); | ||
| console.log('Moon:', planets.find(p => p.id === 'moon')); | ||
|
|
||
| // Calculate Lagna (Ascendant) | ||
| const lagna = await sweph.calculateLagna( | ||
| new Date(), | ||
| { latitude: 27.7, longitude: 85.3, timezone: 5.75 }, | ||
| { ayanamsa: AYANAMSA.LAHIRI } | ||
| ); | ||
|
|
||
| console.log('Ascendant:', lagna.longitude, 'in', sweph.RASHIS[lagna.rasi]); | ||
|
|
||
| // Calculate Moon Phase | ||
| const moonPhase = await sweph.calculateMoonPhase(new Date()); | ||
| console.log('Moon Phase:', moonPhase.phaseName, `(${Math.round(moonPhase.illumination * 100)}%)`); | ||
| } | ||
|
|
||
| main(); |
There was a problem hiding this comment.
The usage example in main() calls new Date() multiple times. For astrological calculations, it's crucial that all computations refer to the exact same moment in time. Using new Date() repeatedly introduces slight time differences between calls, which can lead to inconsistent results.
It's better to define the date once and reuse it for all calculations. For example:
async function main() {
const sweph = await createSweph();
const calculationDate = new Date();
const planets = await sweph.calculatePlanets(calculationDate, ...);
const lagna = await sweph.calculateLagna(calculationDate, ...);
const moonPhase = await sweph.calculateMoonPhase(calculationDate);
// ...
}| ```typescript | ||
| // All 9 Vedic planets | ||
| const planets = await sweph.calculatePlanets(date, { | ||
| ayanamsa: 1, // Lahiri |
There was a problem hiding this comment.
To improve readability and prevent errors from using magic numbers, it's better to use the AYANAMSA constant. This makes the code self-documenting and is consistent with other examples in the README.
| ayanamsa: 1, // Lahiri | |
| ayanamsa: AYANAMSA.LAHIRI, // Lahiri |
| }); | ||
|
|
||
| // Single planet (0=Sun, 1=Moon, 2=Mars, etc.) | ||
| const sun = await sweph.calculatePlanet(0, date, { ayanamsa: 1 }); |
There was a problem hiding this comment.
For consistency and better readability, please use the AYANAMSA constant instead of the magic number 1.
| const sun = await sweph.calculatePlanet(0, date, { ayanamsa: 1 }); | |
| const sun = await sweph.calculatePlanet(0, date, { ayanamsa: AYANAMSA.LAHIRI }); |
| const lagna = await sweph.calculateLagna( | ||
| date, | ||
| { latitude: 27.7, longitude: 85.3 }, | ||
| { ayanamsa: 1 } |
There was a problem hiding this comment.
Using the AYANAMSA constant is preferred over the magic number 1 for clarity and consistency.
| { ayanamsa: 1 } | |
| { ayanamsa: AYANAMSA.LAHIRI } |
|
|
||
| ```typescript | ||
| // Sunrise, Sunset, Solar Noon | ||
| const sunTimes = await sweph.calculateSunTimes(date, location); |
There was a problem hiding this comment.
The location variable is used here without being defined, which will cause an error for users who copy this example. Please define it before use. This applies to the other examples in the 'Sun Calculations' and 'Moon Calculations' sections as well.
For example:
const date = new Date();
const location = { latitude: 27.7, longitude: 85.3 };
const sunTimes = await sweph.calculateSunTimes(date, location);| location: opts?.location ? { | ||
| latitude: opts.location.latitude, | ||
| longitude: opts.location.longitude, | ||
| timezone: opts.location.timezone ?? opts?.timezone ?? 0, | ||
| } : undefined, |
There was a problem hiding this comment.
The timezone property on this location object is redundant. The date is already converted to UTC before being passed to the underlying calculatePlanets function, and the location object itself is only used for azimuth/altitude calculations which do not require a timezone. Removing this makes the code cleaner and less confusing. This also applies to the calculatePlanet method.
location: opts?.location ? {
latitude: opts.location.latitude,
longitude: opts.location.longitude,
} : undefined,
2 participants
BREAKING CHANGE: v2 API is now the recommended interface. Legacy API remains available but is deprecated.