-
Notifications
You must be signed in to change notification settings - Fork 671
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 support for extends to typescript config and babel config parsers #973
Conversation
@@ -67,6 +71,28 @@ export default class TypeScriptConfigParser extends Parser { | |||
try { | |||
config = JSON.parse(fetchEnd.response.body.content); | |||
|
|||
const originalConfig = cloneDeep(config); | |||
|
|||
try { |
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.
Nested try
? Isn't there a way to refactor this?
try { | ||
config = finalConfig(config, resource); | ||
} catch (err) { | ||
if (err.code === ErrorCodes.circular) { |
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.
This checks is also in Babel. Can't it be refactored and maybe added to the Parser
class so it's available in all Parsers
?
@molant I have moved some code to |
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, @alrra do you want to take a look?
@sarvaje looks like there are some errors with the tests:
|
* `resource`: the parsed resource. | ||
* `error`: the error emited parsing the configuration file. | ||
|
||
* `parse::babel-config::error::extends`, of type `BabelConfigInvalidJSON` |
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.
Note: In a conversation with @molant we talked about not having the ...error::not-found
(or similar events) as most of the time just one rule will needed it, and in that case, the rule can have that logic in it.
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.
Can you open an issue for that?
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.
@@ -13,7 +13,7 @@ import { Sonarwhal } from '../../sonarwhal'; | |||
export default class CustomParser extends Parser { | |||
|
|||
public constructor(sonarwhal: Sonarwhal) { | |||
super(sonarwhal); | |||
super(sonarwhal, 'parser-name'); |
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.
For consistency, maybe use:
<parser_name>
@@ -26,7 +26,7 @@ export default class CustomParser extends Parser { | |||
// and maybe leter use a schema if a configuration file or something else | |||
|
|||
// If there's something to share, do it via an event | |||
await this.sonarwhal.emitAsync('customparser::custom', data); | |||
await this.sonarwhal.emitAsync('parse::parser-name::<end | error>', data); |
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.
<parser_name>
?
@@ -40,7 +40,7 @@ Once you have analyzed the resource, the way to share information is via | |||
events (custom or not): | |||
|
|||
```ts | |||
await this.sonarwhal.emitAsync('customparser::custom', data); | |||
await this.sonarwhal.emitAsync('parse::parse-name::<end | error>', data); |
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.
<parser_name>
?
@alrra done! Also, tests should be fixed. |
Fix #943
Pull request checklist
Make sure you:
For non-trivial changes, please make sure you also:
Short description of the change(s)