-
Notifications
You must be signed in to change notification settings - Fork 286
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
ci(github): fix type exports in packages/cactus-common #3649
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ruzell22 Thank you! For your changes to be version controlled you cannot change index.js, you must change index.ts (which is the one that gets compiled to index.js)
d2a08ad
to
ac4f10f
Compare
Primary Changes --------------- 1. Remove packages/cactus-common/hyperledger-cactus-common-*.tgz in ignore paths in get-all-tgz-path.ts file 2. Added the missing LogLevelDesc in index.ts as per attw -f json error log Fixes: hyperledger-cacti#3635 Signed-off-by: ruzell22 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I've re-run failed yarn codegen check to see if it's caused by the PR or not
export const LogLevel = { | ||
TRACE: "TRACE", | ||
trace: "trace", | ||
ERROR: "ERROR", | ||
error: "error", | ||
WARN: "WARN", | ||
warn: "warn", | ||
INFO: "INFO", | ||
info: "info", | ||
DEBUG: "DEBUG", | ||
debug: "debug", | ||
SILENT: "SILENT", | ||
silent: "silent", | ||
} as const; | ||
export type LogLevelDesc = (typeof LogLevel)[keyof typeof LogLevel]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Please do not write code directly in index.ts, it is meant to collect the public API surface exports and that will very noisy if you start writing code directly in it.
- Could you make the LogLevelDesc type a match to what is defined in
packages/cactus-common/node_modules/loglevel/index.d.ts
? There's nothing wrong by the looks of it with what you have here, but it is the safest if we use the exact same definition that the library uses internally to reduce the risk of type compatibility issues later on.
/**
* Log levels
*/
interface LogLevel {
TRACE: 0;
DEBUG: 1;
INFO: 2;
WARN: 3;
ERROR: 4;
SILENT: 5;
}
/**
* Possible log level numbers.
*/
type LogLevelNumbers = LogLevel[keyof LogLevel];
/**
* Possible log level descriptors, may be string, lower or upper case, or number.
*/
type LogLevelDesc = LogLevelNumbers
| 'trace'
| 'debug'
| 'info'
| 'warn'
| 'error'
| 'silent'
| keyof LogLevel;
Commit to be reviewed
ci(github): fix type exports in packages/cactus-common
Fixes: #3635
Pull Request Requirements
upstream/main
branch and squashed into single commit to help maintainers review it more efficient and to avoid spaghetti git commit graphs that obfuscate which commit did exactly what change, when and, why.-s
flag when usinggit commit
command. You may refer to this link for more information.Character Limit
A Must Read for Beginners
For rebasing and squashing, here's a must read guide for beginners.