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 ParseFromReader function #3031

Merged
merged 5 commits into from
Dec 27, 2022
Merged

Add ParseFromReader function #3031

merged 5 commits into from
Dec 27, 2022

Conversation

ShubhamRasal
Copy link
Contributor

  • add parse function to read from the reader interface instead of filepath

@ShubhamRasal ShubhamRasal self-assigned this Dec 13, 2022
@ShubhamRasal ShubhamRasal marked this pull request as draft December 13, 2022 07:33
@ShubhamRasal ShubhamRasal marked this pull request as ready for review December 14, 2022 13:15

// ParseFromReader reads the template from reader
// returns the parsed template
func ParseFromReader(reader io.Reader, options protocols.ExecuterOptions) (*Template, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if it's intentional, but I'd suggest refactoring the existing Parse(filePath string, preprocessor Preprocessor, options protocols.ExecuterOptions) to open the file in filePath and pass the reader to ParseFromReader. This will also ensure that the Variables block is evaluated through the Preprocessor.

Copy link
Member

Choose a reason for hiding this comment

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

I agree, we should make Parse call ParseFromReader so as to reduce duplicate logic

Copy link
Member

@Ice3man543 Ice3man543 left a comment

Choose a reason for hiding this comment

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

requested refactor of functions to reduce duplicate logic

Copy link
Member

@Mzack9999 Mzack9999 left a comment

Choose a reason for hiding this comment

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

  • Minor changes
  • Tests if not already covered by the original Parse

// ParseFromReader reads the template from reader
// returns the parsed template
func ParseFromReader(reader io.Reader, preprocessor Preprocessor, options protocols.ExecuterOptions) (*Template, error) {

Copy link
Member

Choose a reason for hiding this comment

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

extra new line

func ParseFromReader(reader io.Reader, preprocessor Preprocessor, options protocols.ExecuterOptions) (*Template, error) {

template := &Template{}
data, err := ioutil.ReadAll(reader)
Copy link
Member

Choose a reason for hiding this comment

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

ioutil.ReadAll => io.ReadAll

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated it

return nil, err
}

if utils.IsBlank(template.Info.Name) {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't also this logic be in the new ParseFromReader since it performs additional checks and fills other fields that would be missed otherwise?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes.. it is already there in ParseFromReader function.

- update the ioutils to io.ReadAll
- remove extra line
Copy link
Member

@Mzack9999 Mzack9999 left a comment

Choose a reason for hiding this comment

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

I'd recommend to refactor in the following way:

ParseWithPathOrURL loads data from file/url and invoke Parse
..
func ParseWithPathOrURL(filePath string, preprocessor Preprocessor, options protocols.ExecuterOptions) (*Template, error) {
...
	data, err := utils.ReadFromPathOrURL(filePath, options.Catalog)
	if err != nil {
		return nil, err
	}

	template, err := Parse(data, preprocessor, options)
	if err != nil {
		return nil, err
	}
...
ParseWithReader consumes the reader and invoke Parse
..
func ParseWithReader(reader io.Reader, preprocessor Preprocessor, options protocols.ExecuterOptions) (*Template, error) {
	data, err := io.ReadAll(reader)
	if err != nil {
		return nil, err
	}
	return Parse(data, preprocessor, options)
}
...
Parse performs the actual parsing on raw data
..
func Parse(data []byte, preprocessor Preprocessor, options protocols.ExecuterOptions) (*Template, error) {
	template := &Template{}
	...
}
...

@ShubhamRasal ShubhamRasal merged commit 91f6fdf into dev Dec 27, 2022
@ShubhamRasal ShubhamRasal deleted the parse_from_reader branch December 27, 2022 15:17
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.

None yet

3 participants