Skip to content

Fixes when dealing with unusual properties files and other edge-cases. - #67

Closed
nx-eric-jarosch wants to merge 4 commits into
codejive:mainfrom
nx-eric-jarosch:fix-token-insertion
Closed

nx-eric-jarosch wants to merge 4 commits into
codejive:mainfrom
nx-eric-jarosch:fix-token-insertion

Conversation

@nx-eric-jarosch

@nx-eric-jarosch nx-eric-jarosch commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

Properties#put no longer crashes when the last property is at the end of the file and has no value

Example

foo=

or

foo

would previously result in an IllegalStateException when calling Properties#put()


Properties#put inserts the correct amount of blank lines when the file contains no properties but some number of comments.

Example

|   Given   |     Previously     |    Now    |
|===========|====================|===========|
| # header  | # header           | # header  |
|           |                    |           |
|           |                    | put=value |
|           | put=value          |           |
|-----------|--------------------|-----------|
| # header  | # header           | # header  |
|           |                    |           |
| # trailer |                    | put=value |
|           |                    | # trailer |
|           | put=value# trailer |           |

Properties#load no longer swallows the last key if it has no value.

Example

foo=

resulted in a Properties Object with no entries.

See also: #66


Cursor#add skips the inserted item when there are no tokens (matching the behaviour seen when there are tokens)

Previously this would, for example, cause this:

Properties p = new Properties();
Cursor c = p.first();
c.add(new PropertiesParser.Token(PropertiesParser.Type.KEY, "test"));
c.add(new PropertiesParser.Token(PropertiesParser.Type.SEPARATOR, "="));

to store the tokens in the wrong order: tokens = [ SEPARATOR, KEY ]


Cursor#addToken now uses the correct index when checking Cursor#hasToken. Thus the line is inserted in the expected position, instead of at the end of the file.

Example

| Given | Previously |    Now    |
|=======|============|===========|
| foo=  | foo=       | foo=      |
|       |            | put=value |
|       | put=value  |           |


Changes inherited from #65

Corrections of Tests

  • testLoadCrLf
    • added the missing @Test annotation
    • rawValues and rawEntrySet correctly return the \r\n as contained in the test file, but the test expected \n.
  • readAll() currently assumes that Charset.defaultCharset() returns UTF-8.
    That is not the case on my system (or on Windows, in general?). Instead it is windows-1252.
    The fix is to make this assumption explicit.

Resolved Warnings in Tests

  • testRemoveMiddleIterator uses a raw Iterator, the generic type is known to be String at compile time.
  • testInteropPutLoad() does not throw URISyntaxException, removed.

# Conflicts:
#	src/main/java/org/codejive/properties/Properties.java
- Properties#put no longer crashes when the last property is at the end of the file and has no value
- Properties#put inserts the correct amount of blank lines when the file contains no properties but some number of comments.
- Properties#load no longer swallows the last key if it has no value.
- Cursor#add skips the inserted item when there are no tokens (matching the behaviour seen when there are tokens)
- Cursor#addToken now uses the correct index when checking Cursor#hasToken. Thus the line is inserted in the expected position, instead of at the end of the file.
@quintesse

Copy link
Copy Markdown
Collaborator

There's some good stuff in here, but separate issues should ideally be split up into several PRs. Even if it's just a small fix like /r/n in a test. Because it makes it hard to see how one fix/change might affect another.

@nx-eric-jarosch

Copy link
Copy Markdown
Contributor Author

I ended up pulling in #65 because the failing tests made it hard to tell if I broke things, or if things were already broken.

Do you have some tips for how to deal with PRs that build ontop of each other?

@quintesse

Copy link
Copy Markdown
Collaborator

First I'd separate things that don't have anything to do with each other and don't affect one another.

So you could have one PR that says "fixing compiler warnings". It won't affect any other PRs so it doesn't matter if it gets merged first or last or never.

Then I'd start again with a fresh start without any of the changes you just made and make another PR saying something like "Fixing crLf test". Same thing, it doesn't affect any other PRs.

The change in encoding, is an especially good example of having as a separate PR, because even though it's a tiny change it can possibly affect a lot of things. (Encodings always cause trouble) So it might need specific new tests to make sure we didn't break anything with that change.

Now, the moment you want to create a PR that depends on another PR I'd just continue work from that PR and then , when you're done, create a new PR and mention in the description that it needs the other PR to be merged first. BUt honestly I'd only do that when they are complex changes that you really want to separate. If not, just make it a single PR.

In all these cases branches are your friends. With each PR you want to create you first go back to the clean state git checkout main and then first create a new branch for your work, eg git checkout -b fix_warnings. That way you know your work won't interfere with other changes you're making.

@quintesse

Copy link
Copy Markdown
Collaborator

Btw @nx-eric-jarosch I created my own PR to fix the newline issue, because when looking into your changes I found a much bigger problem, there was an actual bug in the code. So I fixed that which also includes your changes (see #69).

@nx-eric-jarosch

nx-eric-jarosch commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor Author

Thanks, I sort-of followed your advice and arrived at this:

This PR is superseded by #71 #72 #73 #75 #76

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