-
Notifications
You must be signed in to change notification settings - Fork 10
test/idea:transformer #5
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
test/idea:transformer #5
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.
I'm going to need explanations on some of the logic changes, before I can approve this PR.
constructor(input: string, options: TransformerOptions = {}) { | ||
this.loader = new FileLoader(options.fs || new NodeFS(), options.cwd); | ||
// Allow custom dependency injection through a constructor | ||
constructor(input: string, options: TransformerOptions = {}, loader?: FileLoader, fs?: NodeFS) { |
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.
What is the purpose of adding the 3rd and 4th argument loader?: FileLoader, fs?: NodeFS
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.
loader?: Fileloader
is for custom instance of the FileLoader
that can handles file loading logic
fs?: NodeFS
it is to allow the Transformer
to be flexible and work with different ways of accessing files
Overall is to allow the Transformer
to work different types of file handling logics
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.
Almost there. Just a few spot fixes.
schema.model = schema.model || {}; | ||
//loop through child types | ||
for (const [ name, model ] of Object.entries(child.model)) { | ||
for (const [name, model] of Object.entries(child.model)) { |
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.
spaces at the start and end of inline arrays
import type { PluginConfig, SchemaConfig } from '@stackpress/idea-parser'; | ||
import type Terminal from './Terminal'; | ||
|
||
//--------------------------------------------------------------------// |
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.
Readd line gap
// Transformer Types | ||
|
||
export type TransformerOptions = { cwd?: string, fs?: FileSystem }; | ||
export type TransformerOptions = {cwd?: string, fs?: FileSystem}; |
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.
space at the start and end of an inline object
//if no plugins defined throw error | ||
if (!this.schema.plugin) { | ||
// The first condition is for missing | ||
// The second condtion is forr not an oject |
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.
typo forr
//if parent isnt final | ||
} | ||
//I remove the else to make the code easier to read and | ||
//understand and to check separete the condition |
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.
Remove commit and code change notes. If someone were to randomly read this file, this would have no context. We should use notes to explain the current logic.
const parent = schema.model[name]; | ||
//if type from child doesn't exist in schema (parent) | ||
if (!parent) { | ||
//I add !parent.mutable to make sure before adding 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.
Remove commit and code change notes. If someone were to randomly read this file, this would have no context. We should use notes to explain the current logic.
// I use to logical Operator OR to combine the 2 condition | ||
// Ensure that the plugin is exist its must be an object | ||
// It is for error handling it's either the condition is true | ||
// it can throw exception or if its not true will stop the tests |
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.
Remove commit and code change notes. If someone were to randomly read this file, this would have no context. We should use notes to explain the current logic.
//if parent isnt final | ||
} | ||
// If the parent is mutable, perform a soft merge | ||
if (parent.mutable ) { |
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.
Remove space after mutable
What is this PR for?
I verify that...
npm run test
with no errors