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

Resolving issue #2541 #2735

Merged
merged 19 commits into from Jun 12, 2019

Conversation

Projects
None yet
3 participants
@alexsaber
Copy link
Contributor

commented May 30, 2019

Basics

  • Short descriptions should be in the release notes (added as entry in
    doc/news/_preparation_next_release.md which contains _(my name)_)
    Please always add something to the the release notes.
  • Longer descriptions should be in documentation or in design decisions.
  • Describe details of how you changed the code in commit messages
    (first line should have module: short statement syntax)
  • References to issues, e.g. close #X, should be in the commit messages.

Checklist

For docu fixes, spell checking, and similar none of these points below
need to be checked.

  • I added unit tests
  • I ran all tests locally and everything went fine
  • [] affected documentation is fixed
  • I added code comments, logging, and assertions (see doc/CODING.md)
  • meta data is updated (e.g. README.md of plugins and doc/METADATA.md)

Review

@markus2330 please review my pull request

Merging

Please add the "ready to merge" label when the build server and you say
that everything is ready to be merged.

alexsaber added some commits Mar 11, 2019

@@ -367,6 +367,14 @@ removed due to:
- New plugin to validate hex formatted colors (e.g. #fff or #abcd) and normalize them to rgba (4294967295 (= 0xffffffff) and 2864434397 (= 0xaabbccdd) respectively). It also has support for named colors according to the [extended color keywords](https://www.w3.org/TR/css-color-3/#svg-color) from CSS3.
_(Philipp Gackstatter)_

### Ini

- new formatting is introduced --> instead of having whitespaces around '=' now the whitespaces are not allowed anymore

This comment has been minimized.

Copy link
@markus2330

markus2330 May 31, 2019

Contributor

The whitespaces should be allowed (it would be an incompatible change to disallow it) but the INI plugin should not write out these whitespaces anymore.

This comment has been minimized.

Copy link
@alexsaber

alexsaber Jun 1, 2019

Author Contributor

@markus2330 Thank you for your comment.
Well, the problem is that I started working on this issue since months and nobody has told me the whitespaces should be allowed. I had another pull request where I pushed my changes I think at the beginning of April. These changes clearly showed that the whitespaces will not be allowed anymore. It would've saved me a lot of time since I hadn't had to fix all those tests due to different formatting. Now I have to reimplement it, probably again fix the tests, etc. I am very much disappointed...

This comment has been minimized.

Copy link
@markus2330

markus2330 Jun 1, 2019

Contributor

I am sorry for the confusion but #2065 clearly says that something additional should be supported and it does not mention that previous working files should be rejected.

I do not think you need to rewrite all tests, nor that your changes are useless. In all tests where the INI file is written you obviously need to remove the spaces, otherwise the files would not be identical after writing.

This comment has been minimized.

Copy link
@alexsaber

alexsaber Jun 10, 2019

Author Contributor

@markus2330
So, after spending many many hours, this is my outcome:

  1. Reading is possible both with and without spaces. The example below will not cause any problems:
one=1
two = 2
  1. Writing is only done without spaces -- if a file had mixed approaches like
one=1
two = 2

after writing for example --

set user/test/three 3

the result would be

one=1
two=2
three=3

Do you confirm this kind of behaviour is what it should be?

@markus2330 Can you please reward my group colleague @e01306821 with some extra points for the class? He provided me support multiple times and deserves nothing less than a "1" in my opinion. Sorry for the off-topic. =)

This comment has been minimized.

Copy link
@markus2330

markus2330 Jun 10, 2019

Contributor

Do you confirm this kind of behaviour is what it should be?

Yes, perfect. This should fix #2541.

Can you please reward my group colleague @e01306821 with some extra points for the class?

Yes. Please try to reflect what you learned in the tutorials/docu.

This comment has been minimized.

Copy link
@alexsaber

alexsaber Jun 10, 2019

Author Contributor

Yes, I will! Thank you very much!

@sanssecours
Copy link
Member

left a comment

Thank you for your contribution 💖. Please,

.


Please refer to the section OS independent below.

## OS independent

This comment has been minimized.

Copy link
@sanssecours

sanssecours May 31, 2019

Member
Suggested change
## OS independent
## OS Independent

Please use title-case for headers.

mountpoint = tutorial.dump \
infos/plugins = dump validation \
mountpoint=tutorial.dump \
infos/plugins=dump validation \
\

This comment has been minimized.

Copy link
@sanssecours

sanssecours May 31, 2019

Member

Please also remove two whitespaces in line 231 and 232 to keep the formatting consistent.

@@ -38,6 +38,9 @@ static int iniCmpOrder (const void * a, const void * b);
#define DEFAULT_DELIMITER '='
#define DEFAULT_COMMENT_CHAR '#'

char * const ININAME_DELIM_SPECIAL_MASK = "\"%s\"%c";

This comment has been minimized.

Copy link
@sanssecours

sanssecours May 31, 2019

Member
Suggested change
char * const ININAME_DELIM_SPECIAL_MASK = "\"%s\"%c";
char const * const ININAME_DELIM_SPECIAL_MASK = "\"%s\"%c";

You can also specify that the data (not only the pointer to the data) will not be changed here.

example of old formatting: key1 = value1
example of new formatting: key1=value1

. _(Oleksandr Shabelnyk)_

This comment has been minimized.

Copy link
@sanssecours

sanssecours May 31, 2019

Member

Please fix the indentation of your name (exactly two spaces before the dot). Otherwise your name will show up in code style:

Release

. If you want to know why that is the case, please take a look at the Markdown specification.

@@ -38,6 +38,9 @@ static int iniCmpOrder (const void * a, const void * b);
#define DEFAULT_DELIMITER '='
#define DEFAULT_COMMENT_CHAR '#'

char * const ININAME_DELIM_SPECIAL_MASK = "\"%s\"%c";
char * const ININAME_DELIM_MASK = "%s%c";

This comment has been minimized.

Copy link
@sanssecours

sanssecours May 31, 2019

Member
Suggested change
char * const ININAME_DELIM_MASK = "%s%c";
char const * const ININAME_DELIM_MASK = "%s%c";
@@ -388,9 +388,16 @@ removed due to:
- New plugin to validate hex formatted colors (e.g. #fff or #abcd) and normalize them to rgba (4294967295 (= 0xffffffff) and 2864434397 (= 0xaabbccdd) respectively). It also has support for named colors according to the [extended color keywords](https://www.w3.org/TR/css-color-3/#svg-color) from CSS3.
_(Philipp Gackstatter)_

<<<<<<< HEAD

This comment has been minimized.

Copy link
@sanssecours

sanssecours Jun 11, 2019

Member

Looks like you had a merge conflict and forgot to remove the conflict markers:

Suggested change
<<<<<<< HEAD

.

This comment has been minimized.

Copy link
@alexsaber

alexsaber Jun 11, 2019

Author Contributor

Oh, sorry, I thought I removed the markers, but it seems like I didn't...


- Plugin writes to ini files without spaces around '=' anymore. Reading is still possible with and without spaces.
_(Oleksandr Shabelnyk)_
=======

This comment has been minimized.

Copy link
@sanssecours

sanssecours Jun 11, 2019

Member
Suggested change
=======
### macaddr

- Added a plugin to handle MAC addresses. `kdbGet` converts a MAC address into a decimal 64-bit integer (with the most significant 16 bits always set to 0), if the format is supported. `kdbSet` restores the converted values back to there original form. _(Thomas Bretterbauer)_
>>>>>>> 3624c12fb04d6d370aaee311da2f913fa140e2f1

This comment has been minimized.

Copy link
@sanssecours

sanssecours Jun 11, 2019

Member
Suggested change
>>>>>>> 3624c12fb04d6d370aaee311da2f913fa140e2f1

@markus2330 markus2330 merged commit d64deec into ElektraInitiative:master Jun 12, 2019

10 of 12 checks passed

LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
LGTM analysis: C/C++ No new or fixed alerts
Details
bsd FreeBSD:freebsd-11-2-release-amd64 Task Summary
Details
bsd FreeBSD:freebsd-12-0-release-amd64 Task Summary
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
linux Task Summary
Details
mac Task Summary
Details
mac ASAN_OPTIONS:detect_leaks=1 BINDINGS:cpp ENABLE_ASAN:ON TOOLS:kdb Task Summary
Details
mac BUILD_FULL:ON BUILD_SHARED:OFF Task Summary
Details
mac KDB_DB_FILE:default.mmap KDB_DB_INIT:elektra.mmap KDB_DEFAULT_STORAGE:mmapstorage Task Summary
Details
@markus2330

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.