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

Bufix: pass settings to from Radix to AspectCalculator #71

Merged
merged 2 commits into from
Mar 21, 2024

Conversation

harlantwood
Copy link
Contributor

@harlantwood harlantwood commented Mar 11, 2024

Without this, AspectCalculator will always fall back to default settings, in commented line below:

 class AspectCalculator {
  settings: Partial<Settings>
  toPoints: Points
  context: this
  constructor (toPoints: Points, settings?: Partial<Settings>) {
    if (toPoints == null) {
      throw new Error('Param \'toPoint\' must not be empty.')
    }

    this.settings = settings ?? {}
    this.settings.ASPECTS = settings?.ASPECTS ?? DEFAULT_ASPECTS // <--- `Radix` will always use `DEFAULT_ASPECTS` (before this fix)

    this.toPoints = toPoints

    this.context = this
  }

@eve0803
Copy link

eve0803 commented Mar 11, 2024

Can you write a case study of passing in ASPECTS settings?

@harlantwood
Copy link
Contributor Author

Can you write a case study of passing in ASPECTS settings?

Do you mean write a test? Or something else?

@harlantwood harlantwood force-pushed the pass-settings-to-aspectcalc branch from b3cf0cd to c080f08 Compare March 12, 2024 00:35
@harlantwood harlantwood force-pushed the pass-settings-to-aspectcalc branch from c080f08 to 2791511 Compare March 12, 2024 07:37
@harlantwood
Copy link
Contributor Author

Added a test, which fails before the bugfix, but passes now that it's added:

Screen Shot 2024-03-11 at 9 41 06 PM

Copy link
Contributor

@afucher afucher left a comment

Choose a reason for hiding this comment

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

LGTM

@afucher afucher merged commit 72d8018 into AstroDraw:main Mar 21, 2024
3 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.

3 participants