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

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

Merged
merged 1 commit into from
Jun 15, 2021

Conversation

bwplotka
Copy link
Owner

Signed-off-by: Bartlomiej Plotka [email protected]

@bwplotka bwplotka requested a review from saswatamcode June 11, 2021 19:23
Copy link
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
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
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
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.

panic("should never get here")
}

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

func genGo(ctx context.Context, moduleRoot string, typePath string) ([]byte, error) {
Copy link
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
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
Collaborator

Choose a reason for hiding this comment

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

Oh okay!

@@ -40,3 +36,8 @@ test output3

echo "test output3"
```

```yaml mdox-exec="bash ./testdata/out2.sh --name=queryfrontend.InMemoryResponseCacheConfig"
Copy link
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
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