[GitHub] [geode-native] mmartell opened pull request #441: Geode 4340 authinit

["mmartell (GitHub)" <gi...@apache.org>]

This PR adds a new cpp authInit example and simplifies the building and running of the examples.

[ Full content available at: https://github.com/apache/geode-native/pull/441 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] mreddington commented on pull request #441: Geode 4340 authinit

["mreddington (GitHub)" <gi...@apache.org>]

The documentation was all over the place and almost none of it had any console tag associated with it. The *one* I did see that on didn't format the text any different than a gray box just like all the rest. If it had formatted like you imply, I would have kept with the backticks. If it turns out Github at least does properly format the console tag, I'll add it.

But backticks are otherwise mysterious, indentation is intuitive. Markdown has indent enabled formatting for this very purpose - it's no coincidence that the right thing occurs when you properly indent in markdown.

[ Full content available at: https://github.com/apache/geode-native/pull/441 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] pivotal-jbarrett commented on pull request #441: Geode 4340 authinit

["pivotal-jbarrett (GitHub)" <gi...@apache.org>]

Importing full namespace is frowned upon in the Google C++ Style Guide.

[ Full content available at: https://github.com/apache/geode-native/pull/441 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] mmartell commented on pull request #441: Geode 4340 authinit

["mmartell (GitHub)" <gi...@apache.org>]

Although indentation is somewhat easier to read in the markdown, we're usually looking at the rendered output. So readability may be less important than being consistent in our docs. We seem to mostly use back tics. Also, I like the color coding afforded by using using console.

WINNER: Back tics, with console attribute.

[ Full content available at: https://github.com/apache/geode-native/pull/441 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] pivotal-jbarrett commented on pull request #441: Geode 4340 authinit

["pivotal-jbarrett (GitHub)" <gi...@apache.org>]

Why not just copy them all all the time?

[ Full content available at: https://github.com/apache/geode-native/pull/441 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] mmartell commented on pull request #441: Geode 4340 authinit

["mmartell (GitHub)" <gi...@apache.org>]

WINNER: Copy both types of scripts. (I want powershell everywhere.)

Good catch on the Win64. Switching to WIN32.

[ Full content available at: https://github.com/apache/geode-native/pull/441 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] mmartell commented on pull request #441: Geode 4340 authinit

["mmartell (GitHub)" <gi...@apache.org>]

Thy will be done. Frankly, this is a bad behavior I've found disagreeable and I'm glad you call it out. [Matt]

[ Full content available at: https://github.com/apache/geode-native/pull/441 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] mmartell commented on pull request #441: Geode 4340 authinit

["mmartell (GitHub)" <gi...@apache.org>]

Thy will be done. [Matt]

[ Full content available at: https://github.com/apache/geode-native/pull/441 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] pivotal-jbarrett commented on pull request #441: Geode 4340 authinit

["pivotal-jbarrett (GitHub)" <gi...@apache.org>]

My point on the scripts is that we have plenty of examples of installing bat and bash scripts together. The effort to distinguish which platform to copy which script is really not necessary. Especially when you consider the proliferation of bash on windows now too.

[ Full content available at: https://github.com/apache/geode-native/pull/441 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] pivotal-jbarrett commented on pull request #441: Geode 4340 authinit

["pivotal-jbarrett (GitHub)" <gi...@apache.org>]

This won’t work if I make a copy of the examples directory. Use findGeode and copy the dll.

[ Full content available at: https://github.com/apache/geode-native/pull/441 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] pivotal-jbarrett commented on pull request #441: Geode 4340 authinit

["pivotal-jbarrett (GitHub)" <gi...@apache.org>]

Y’all are confusing generators with platforms. If I’m on windows platform you want dll’s and ps1 files. I can use many generators on Windows and most of them won’t have the word Win64 in them. Only the current VS generators do. VS 2019 generator does not. Use platform checks for platform stuff. If you’re doing generator checks you are probably not doing something right. Their use should be Baer very infrequent.


[ Full content available at: https://github.com/apache/geode-native/pull/441 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] mmartell commented on pull request #441: Geode 4340 authinit

["mmartell (GitHub)" <gi...@apache.org>]

Less is more. The rendered output looks exactly the same. Also, it's not really code, and it makes the markdown clearer because we know the indentation is part of the list item.

[ Full content available at: https://github.com/apache/geode-native/pull/441 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] mmartell commented on pull request #441: Geode 4340 authinit

["mmartell (GitHub)" <gi...@apache.org>]

This was already switched to use platform check (WIN32).

[ Full content available at: https://github.com/apache/geode-native/pull/441 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] mmartell commented on pull request #441: Geode 4340 authinit

["mmartell (GitHub)" <gi...@apache.org>]

What are you going to do in Bash on Linux with a ps1 script? What are you going to do with a bash script in powershell? What will the client think when we present them with scripts that don't natively exist on their given platform? We would risk confusing the client. [Matt]

Are you thinking of Linux users that prefer PowershellCore or Windows users that use Ubuntu on Windows? [Mike]

[ Full content available at: https://github.com/apache/geode-native/pull/441 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] pivotal-jbarrett commented on pull request #441: Geode 4340 authinit

["pivotal-jbarrett (GitHub)" <gi...@apache.org>]

Less is more? How are 6 backticks suddenly more than N x 4 spaces, where N is the number of lines > 1? 

Formats the same? Not sure where it go dropped but this code blocks had a language of console for proper highlighting. 
``````
```console
$ command --args
```
``````
produces
```console
$ command --args
```

[ Full content available at: https://github.com/apache/geode-native/pull/441 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] pivotal-jbarrett commented on pull request #441: Geode 4340 authinit

["pivotal-jbarrett (GitHub)" <gi...@apache.org>]

No I am saying that you conditional pointless for something so meaningless. Additionally the conditional is is matching the generator for Win64, there is no such generator. What if I use NMake on windows, this won't match.


[ Full content available at: https://github.com/apache/geode-native/pull/441 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] pivotal-jbarrett commented on pull request #441: Geode 4340 authinit

["pivotal-jbarrett (GitHub)" <gi...@apache.org>]

What is the rational for this formatting change. The code block formatting is more appropriate than the fixed width indention. 

[ Full content available at: https://github.com/apache/geode-native/pull/441 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] mmartell closed pull request #441: GEODE-4340: authinit

["mmartell (GitHub)" <gi...@apache.org>]

[ pull request closed by mmartell ]

[ Full content available at: https://github.com/apache/geode-native/pull/441 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] mmartell commented on pull request #441: Geode 4340 authinit

["mmartell (GitHub)" <gi...@apache.org>]

Good call. Changed to using the find module for geode-native to locate the dll.

[ Full content available at: https://github.com/apache/geode-native/pull/441 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] mreddington commented on pull request #441: Geode 4340 authinit

["mreddington (GitHub)" <gi...@apache.org>]

How is installing the only script that will intuitively and natively execute on that platform meaningless? I'm sure a *nix developer is going to see a ps1 script and go "Oh boy! An excuse to install powershell!"

As far as detecting windows, fair. What would be more appropriate? Win32 or Win64? Because we seem to use both, and I never know which is going to be defined, especially since we in no way support 32 bit Windows (yet Windows dev loves to hang onto the past).

[ Full content available at: https://github.com/apache/geode-native/pull/441 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode-native] pivotal-jbarrett commented on pull request #441: Geode 4340 authinit

["pivotal-jbarrett (GitHub)" <gi...@apache.org>]

I'd avoid importing types in headers this way. This moves the type into the default namespace an any unit that includes this header now.

[ Full content available at: https://github.com/apache/geode-native/pull/441 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org