Skip to content

Fixed mdox gen errors on command with = inside (common case).#41

Merged
bwplotka merged 1 commit intomainfrom
mdoxgen-fix
Jun 15, 2021
Merged

Fixed mdox gen errors on command with = inside (common case).#41
bwplotka merged 1 commit intomainfrom
mdoxgen-fix

Conversation

@bwplotka
Copy link
Copy Markdown
Owner

Signed-off-by: Bartlomiej Plotka bwplotka@gmail.com

Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
@bwplotka bwplotka requested a review from saswatamcode June 11, 2021 19:23
Copy link
Copy Markdown
Collaborator

@saswatamcode saswatamcode left a comment

Choose a reason for hiding this comment

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

This looks awesome! ✨ Just some suggestions!

val := strings.Split(field, "=")
val := []string{field}
if i := strings.Index(field, "="); i != -1 {
val = []string{field[:i], field[i+1:]}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Awesome! Now it is only split at first "=". But maybe instead of using Index this can be done with something like,

val := strings.SplitN(field, "=", 2)

This also would only split at first "=" and return slice of length 2. WDYT?🙂

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Tried to use it, but found it confusing why 2 not 1.. (: will try thanks!

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think 2 is the number of substrings returned...but yes it is not intuitive. This can be skipped then I guess.


func (t *genCodeBlockTransformer) Close(ctx mdformatter.SourceContext) error { return nil }

func genGo(ctx context.Context, moduleRoot string, typePath string) ([]byte, error) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I thought this was to be done as mentioned in #23. Do you have something else in mind? 🙂

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

I think this part is not clearly understood yet. We don't know yet how it should look.

This snippet was mainly for the beginning to show you and Uche how we might want to do it.

Normally it's not the best to maintain code which is not used (YAGNI) so killed it (:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Oh okay!

echo "test output3"
```

```yaml mdox-exec="bash ./testdata/out2.sh --name=queryfrontend.InMemoryResponseCacheConfig"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nice testcase! 💪🏼

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Yea. It was failing for us before

@bwplotka bwplotka merged commit 06888e7 into main Jun 15, 2021
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.

2 participants