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

Add types for cds.utils.colors #263

Merged
merged 8 commits into from
Oct 2, 2024
Merged

Add types for cds.utils.colors #263

merged 8 commits into from
Oct 2, 2024

Conversation

swaldmann
Copy link
Contributor

@swaldmann swaldmann commented Oct 2, 2024

Requires cap-js/docs#1285 for the @see link to work.

See cap/cds/pull/4656

@swaldmann swaldmann requested review from daogrady and chgeo and removed request for daogrady October 2, 2024 09:25
@swaldmann swaldmann marked this pull request as ready for review October 2, 2024 09:25
@daogrady
Copy link
Contributor

daogrady commented Oct 2, 2024

Thanks for this contribution! Seeing that the colour strings seem to be used as both primary and background-colour, would pulling them out into a separate type be desirable?

Alternatively just as a dictionary.

@chgeo
Copy link
Member

chgeo commented Oct 2, 2024

Since which sap/cds version is this available? Should document this.
@daogrady do we have a @since tag or so?

@daogrady
Copy link
Contributor

daogrady commented Oct 2, 2024

@since is valid in JSDoc, yes. Possibly not in TSDoc, but I don't see why we should not use it anyway.

@swaldmann
Copy link
Contributor Author

Since which sap/cds version is this available?

Was merged just a week ago, so >= 8.3.0.

should document this

Yep :) cap-js/docs#1285

seeing that the colour strings seem to be used as both primary and background-colour, would pulling them out into a separate type be desirable?

Two reasons why I kept it separate:

  • There are some small exceptions, e.g. GRAY is available as a foreground color, but not as a background color
  • While the key names are the same the constant values of RED ('\x1b[31m') and bg.RED ('\x1b[41m') are different
    → sharing a type/dict seems to imply a reuse that's semantically not there

@daogrady
Copy link
Contributor

daogrady commented Oct 2, 2024

There are some small exceptions, e.g. GRAY is available as a foreground color, but not as a background color

I see, then maybe

type Colors = {
  BLUE: string,
  GREEN: string,
  ...
}

type colors: Colors & {
  ITALIC: string,
  ...
  GRAY: string,  // exclusive to foreground colors
  bg: Colors & {
      DEFAULT: string
  }
}

?

@swaldmann
Copy link
Contributor Author

I just found out I could type the values more explicitly using <constant escape sequence> | '', as those are the only values they can take on. This is a tighter typing (which is good!) but it doesn't allow us to reuse the values from a type/dict.

Copy link
Contributor

@daogrady daogrady left a comment

Choose a reason for hiding this comment

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

That looks indeed better. Still not super stoked about this looming typo-catastrophe, but I'm probably blowing this out of proportion. 🙂

@swaldmann swaldmann enabled auto-merge (squash) October 2, 2024 12:57
@swaldmann swaldmann merged commit 9571d15 into main Oct 2, 2024
8 checks passed
@swaldmann swaldmann deleted the colors branch October 2, 2024 12:59
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