-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Added GetEnvironmentStrings to 'com.sun.jna.platform.win32.Kernel32' #434
Added GetEnvironmentStrings to 'com.sun.jna.platform.win32.Kernel32' #434
Conversation
04bfbc1
to
054394a
Compare
for (Entry<String, String> entry : environment.entrySet()) { | ||
if (entry.getValue() != null) { | ||
out.append(entry.getKey() + "=" + entry.getValue() + "\0"); | ||
String key=entry.getKey(), value=entry.getValue(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Space/tab? Make sure there're spaces around =
please, so String key = entry.getKey(), value = entry.getValue();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been the style of changes I submitted in the past and no one remarked about it. I have taken notice and will make an effort to have future submissions adhere to this style. As far as this submission goes, I would ask that you accept this change as-is, since back-formatting all the code I wrote with the spaces around the assignment operator would be tedious (I know Eclipse has a formatting option but I am afraid that other formatting issues will appear that may not be the way you like it - unless you can send me the exact configuration file for the Eclipse style).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I overlooked this before, it's not that important, but it looks sloppy to future developers - I'll take it, but I would appreciate if you could look into auto-formatting this and others (I don't have the exact Eclipse configuration, sorry), and maybe run something automatic for a future commit. We would like to make JNA look like it was written by 1 person.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duly noted (though I disagree with looks sloppy
...) - I do agree with make JNA look like it was written by 1 person
and will adhere to this code style in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good work, btw, really appreciate more contributions around win32 mappings! |
|
I would say the next big thing is fixing some of the tests that are inconsistently failing, https://github.com/twall/jna/labels/unit%20tests. Then #282, #258 and #251 that were PRed but either didn't have tests or issues. |
054394a
to
16b72e5
Compare
Done - renamed as discussed and pushed |
As far as #251 is concerned, it should be re-worked on my (previous) submission regarding the console API . |
assertCallSucceeded("Clean up variable", Kernel32.INSTANCE.SetEnvironmentVariable(name, null)); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have split this into Kernel32EnvironmentTest and Kernel32UtilEnvironmentTest, but I'll accept it as is.
Added GetEnvironmentStrings to 'com.sun.jna.platform.win32.Kernel32'
Including code to parse the resulting Pointer in Kernel32Utils (wait a moment for the CHANGES.md - the chicken and egg issue...)