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

Pretty print fact values #176

Merged
merged 2 commits into from
Jan 26, 2023
Merged

Pretty print fact values #176

merged 2 commits into from
Jan 26, 2023

Conversation

dottorblaster
Copy link
Contributor

This PR introduces pretty printing for fact values into the CLI, formatting maps and lists as Rhai object maps and arrays.

@dottorblaster dottorblaster force-pushed the rhai-print-fact-gathering branch 3 times, most recently from 9d62a86 to 3141a65 Compare January 26, 2023 08:53
@dottorblaster dottorblaster marked this pull request as ready for review January 26, 2023 08:53
@angelabriel
Copy link

❤️ Hey awesome, I really like this idea. Makes it so much easier to take the results from the gatherers to the playground to try out some 'wild' ideas 😃

@dottorblaster
Copy link
Contributor Author

@angelabriel original idea is by @fabriziosestito, I'm just an executor :D All the love goes to him 😄

@@ -98,6 +102,10 @@ func (v *FactValueString) AsInterface() interface{} {
return v.Value
}

func (v *FactValueString) PrettyPrint() (string, error) {
Copy link
Member

Choose a reason for hiding this comment

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

leftover?

@@ -36,6 +40,16 @@ func (e *FactGatheringError) Wrap(msg string) *FactGatheringError {
}
}

func (e *Fact) PrettyPrint() (string, error) {
Copy link
Member

Choose a reason for hiding this comment

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

maybe we should cal this Prettify as well since it does not print

suite.Run(t, new(FactsGatheredTestSuite))
}

func (suite *FactsGatheredTestSuite) TestFactValuePrettify() {
Copy link
Member

Choose a reason for hiding this comment

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

this is testing FactsPrettyPrint not FactValuePrettify

@dottorblaster dottorblaster force-pushed the rhai-print-fact-gathering branch 3 times, most recently from 14cdd84 to feb8a54 Compare January 26, 2023 09:44
Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

It looks great man!
Nice work

func (e *Fact) Prettify() (string, error) {
prettifiedValue, err := Prettify(e.Value)
if err != nil {
return "", errors.Wrap(err, "Error pretty-printing fact value data")
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe better the error being Error prettifying fact value data

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 👍

@@ -192,3 +196,28 @@ func getValue(fact FactValue, values []string) (FactValue, error) {
return value, nil
}
}

func Prettify(fact FactValue) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this maybe be a private function?
And it could've been perfectly a struct function as well:
func (f *FactValue) Prettify() (string, error)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the same way I tried to pursue at the beginning, but it requires a method to be implemented for each implementation of FactValue. Instead, this way the function just works ™️ with every concrete type

Copy link
Member

Choose a reason for hiding this comment

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

@arbulu89 you cannot add methods to an interface

Copy link
Member

Choose a reason for hiding this comment

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

The function is better, you could achieve a semi functional result with a dedicated struct with the method implemented and then embedding with some hassle in every struct you want to prettify

But honestly I don't think it's very ideomatic, seems like some sort of oop abstract superclass with some default methods

dottorblaster and others added 2 commits January 26, 2023 11:09
Co-authored-by: Fabrizio Sestito <fabrizio.sestito@suse.com>
@fabriziosestito fabriziosestito merged commit f855095 into main Jan 26, 2023
@fabriziosestito fabriziosestito deleted the rhai-print-fact-gathering branch January 26, 2023 12:29
@arbulu89 arbulu89 added the enhancement New feature or request label Apr 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

5 participants