logchange icon indicating copy to clipboard operation
logchange copied to clipboard

verbose message for incorrect value errors

Open NikodemMarek opened this issue 1 year ago • 5 comments

related to this https://github.com/logchange/logchange/pull/282#discussion_r1660187287

NikodemMarek avatar Sep 27 '24 07:09 NikodemMarek

The change introduced in PR #282 is causing the issue described in issue #317. Specifically, the problem lies in this part of the code:

    private List<ChangelogEntry> getEntries(File versionDirectory) {
        return getEntriesFiles(versionDirectory)
                .sequential()
                .map((file) -> {
                    try {
                        return YMLChangelogEntry.of(getEntryInputStream(file), file.getPath());
                    } catch (YMLChangelogEntryConfigException e) {
                        log.warning(e.getMessage());
                        return null;
                    }
                })
                .filter(Objects::nonNull)
                .map(YMLChangelogEntry::to)
                .collect(Collectors.toList());
    }

In my opinion, it's better not to handle this exception and instead allow the process to crash, so the user is immediately aware of the issue and can fix the problematic file. The current approach silently skips the invalid file, which may lead to unnoticed errors.

I suggest reverting the code to:

    private List<ChangelogEntry> getEntries(File versionDirectory) {
        return getEntriesFiles(versionDirectory)
                .sequential()
                .map((file) -> YMLChangelogEntry.of(getEntryInputStream(file), file.getPath()))
                .map(YMLChangelogEntry::to)
                .collect(Collectors.toList());
    }

This change will make the error handling more transparent and help users address configuration issues promptly. @NikodemMarek @marwin1991

witx98 avatar Sep 27 '24 18:09 witx98

@witx98 Is there an option, to throw exception after running all files ?

Now IMO process will stop on the first wrong file, but I think it would be nice to have information about all wrong entries.

Probably:

                .map((file) -> {
                    try {
                        return YMLChangelogEntry.of(getEntryInputStream(file), file.getPath());
                    } catch (YMLChangelogEntryConfigException e) {
                        log.warning(e.getMessage());
                        return null;
                    }
                })

will brake lint command ? @NikodemMarek

marwin1991 avatar Sep 27 '24 18:09 marwin1991

I believe solution laki this should address the requirement to collect errors from all files before throwing an exception:

    private List<ChangelogEntry> getEntries(File versionDirectory) {
        List<Exception> exceptions = new ArrayList<>();

        List<ChangelogEntry> entries = getEntriesFiles(versionDirectory)
                .sequential()
                .map((file) -> {
                    try {
                        return YMLChangelogEntry.of(getEntryInputStream(file), file.getPath());
                    } catch (YMLChangelogEntryConfigException e) {
                        exceptions.add(e);
                        return null;
                    }
                })
                .filter(Objects::nonNull)
                .map(YMLChangelogEntry::to)
                .collect(Collectors.toList());

        if (!exceptions.isEmpty()) {
            throw new YMLChangelogException(exceptions);
        }

        return entries;
    }
   public class YMLChangelogException extends RuntimeException {
   public YMLChangelogException(List<Exception> exceptions) {
       super(toString(exceptions));
   }

   private static String toString(List<Exception> exceptions) {
       StringBuilder sb = new StringBuilder();
       sb.append("Errors found:\n");
       exceptions.forEach(exception -> sb.append(exception.getMessage()).append("\n"));
       return sb.toString();
   }
}

@marwin1991

witx98 avatar Sep 27 '24 19:09 witx98

@witx98 exactly!

marwin1991 avatar Sep 28 '24 07:09 marwin1991

great suggestion @witx98, I'm gonna apply it when I get the time

NikodemMarek avatar Sep 28 '24 07:09 NikodemMarek