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

Update docs #24

Merged
merged 4 commits into from
Jan 20, 2021
Merged

Update docs #24

merged 4 commits into from
Jan 20, 2021

Conversation

a4z
Copy link
Contributor

@a4z a4z commented Jan 20, 2021

Add cli doc for recent change , #14 / #22
Check that docs for #20 / #21 is in place
Improve Identifyer Style section , for #11

Add cli doc for recent change , cross-language-cpp#14 / cross-language-cpp#22
Check that docs for cross-language-cpp#20 / cross-language-cpp#21 is in place
Improve Identifyer Style section
@a4z a4z requested a review from jothepro January 20, 2021 10:00
@a4z
Copy link
Contributor Author

a4z commented Jan 20, 2021

I wonder if there is a way to preview the docs local, especially when playing around with mkdocs specials, like the
!!! note section

@richey-v
Copy link

I wonder if there is a way to preview the docs local, especially when playing around with mkdocs specials, like the
!!! note section

I've used Python mkdocs locally before on a project. That project was hosted on a local Gitlab instance (not github) so I spent some time ensuring my MarkDown was compatible and rendered as intended with both the Gitlab and mkdocs rendering.

@jothepro
Copy link
Contributor

jothepro commented Jan 20, 2021

you can preview the docs by running mkdocs serve.
This will render the docs with the default mkdocs theme.

The admonition blocks (!!! note) require a special plugin to be activated. So this will not be previewed correctly without further configuration

I opened an issue to improve the preview-ability in each repository. Thanks for your feedback, @a4z

Copy link
Contributor

@jothepro jothepro left a comment

Choose a reason for hiding this comment

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

Looks great so far! But I have a few optional questions and ideas for improvement.

docs/cli-usage.md Outdated Show resolved Hide resolved
docs/cli-usage.md Outdated Show resolved Hide resolved
```
As you see, _VALUE_ is now in upper case letters.

If you use `--ident-java-enum foo_bar` then _Value_ will not be transformed into upper case and the following Java code will be generated.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is so weird 🙈
If changing the Identifier names would not potentially break so many builds I'd prefer more speaking names for them:

  • camelCase
  • TrainCase
  • snake_case
  • none <== This is what foo_bar currently is.
  • ...

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about renaming them? This just came to my mind a few days ago. I don't see it in the scope of this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

might still be a breaking change.
we could add none as a keyword to start a smooth transition process ....

Copy link
Contributor

Choose a reason for hiding this comment

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

just want to talk with you about this on a theoretical basis. :P I'm aware that this is a major breaking change.

Copy link
Contributor

@jothepro jothepro left a comment

Choose a reason for hiding this comment

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

Approved given that the empty trailing row in the ident-style parameter table is removed. 👍

Co-authored-by: jothepro <[email protected]>
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.

3 participants