Cli for consistency result by priyanshu16095 · Pull Request #12475 · JabRef/jabref

priyanshu16095 · 2025-02-09T17:15:01Z

Fixes #11984

This PR adds a CLI to ensure consistency among BibTeX entries, with --check-consistency FILE[FORMAT], supporting txt, csv and console based output.

Mandatory checks

I own the copyright of the code submitted and I licence it under the MIT license
Change in CHANGELOG.md described in a way that is understandable for the average user (if change is visible to the user)
Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)
Screenshots added in PR description (for UI changes)
Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

github-actions

JUnit tests are failing. In the area "Some checks were not successful", locate "Tests / Unit tests (pull_request)" and click on "Details". This brings you to the test output.

You can then run these tests in IntelliJ to reproduce the failing tests locally. We offer a quick test running howto in the section Final build system checks in our setup guide.

github-actions

JUnit tests are failing. In the area "Some checks were not successful", locate "Tests / Unit tests (pull_request)" and click on "Details". This brings you to the test output.

You can then run these tests in IntelliJ to reproduce the failing tests locally. We offer a quick test running howto in the section Final build system checks in our setup guide.

priyanshu16095 · 2025-02-09T17:47:32Z

Writing the output to the console is left.

koppor

Looks OK. Some improvement suggestions inside.

Think of "knobless" tool - a tool should work out of the box without configuration. If I as user say "--check-consistency test.bib", it should just check and output it to System.out.

koppor · 2025-02-09T19:37:54Z

+            if (outputFormat.isEmpty()) {
+                writeFindings(result);
+                return;
+            }


Just assume txt as output format - and write it to System.out - Should be possible with new OutputStreamWriter(System.out).

Comment still relevant.

koppor · 2025-02-09T19:38:37Z

+    public void writeFindings(BibliographyConsistencyCheck.Result result) {
+        System.out.print(Localization.lang("Field Presence Consistency Check Result"));
+        System.out.print("\n\n");
+
+        if (result.entryTypeToResultMap().isEmpty()) {
+            System.out.println("No errors found.\n");
+            return;
+        }
+
+        System.out.print("\n");
+        System.out.print("%s | %s\n".formatted("x", Localization.lang("required field is present")));
+        System.out.print("%s | %s\n".formatted("o", Localization.lang("optional field is present")));
+        System.out.print("%s | %s\n".formatted("?", Localization.lang("unknown field is present")));
+        System.out.print("%s | %s\n".formatted("-", Localization.lang("field is absent")));
    }


Remove this method. Using org.jabref.logic.quality.consistency.BibliographyConsistencyCheckResultTxtWriter is enough.

koppor · 2025-02-09T19:40:49Z

+
+    @Test
+    void checkConsistencyOutputFormatOption() throws Exception {
+        CliOptions cli = new CliOptions(new String[] {"--check-consistency", "jabref-authors.bib", "--check-consistency-output-format", "CSV"});


Use lower letter for parameters - upper case letters seems so 1990s.

koppor · 2025-02-10T08:42:00Z

+        if (fileName == null) {
+            System.out.println(Localization.lang("No file specified for consistency check."));
+            return;
+        }


Move this after line 305, so that you "exit early" / fail fast.

The "outputformat" is not read in this if - and this not necessary.

koppor · 2025-02-10T08:42:33Z

+            if (outputFormat.isEmpty()) {
+                writeFindings(result);
+                return;
+            }


Comment still relevant.

github-actions

JUnit tests are failing. In the area "Some checks were not successful", locate "Tests / Unit tests (pull_request)" and click on "Details". This brings you to the test output.

You can then run these tests in IntelliJ to reproduce the failing tests locally. We offer a quick test running howto in the section Final build system checks in our setup guide.

priyanshu16095 · 2025-02-10T10:48:57Z

+
+    @Test
+    void checkConsistencyOutputFormatOption() throws Exception {
+        CliOptions cli = new CliOptions(new String[] {"--check-consistency", "jabref-authors.bib", "--output-format", "csv"});


I renamed the --check-consistency-output-format to --output-format. Is it OK?

On the one hand, we also have exporters with an output format. On the other hand, their syntax is very oudated.

The whole command line should be reworked.

We need to do sub commands instead of --check-consistency. --> https://clig.dev/#the-basics

But maybe, this is for 7.0 instead of 6.0 :) (But I think, we can do this in 6.0)

TL;DR: This is OK.

I think we decided a few months ago on picocli as a future replacement for the apache command line parser. We should introduce it with a new major release, and I would then opt for a full review of all cli options.

github-actions

JUnit tests are failing. In the area "Some checks were not successful", locate "Tests / Unit tests (pull_request)" and click on "Details". This brings you to the test output.

You can then run these tests in IntelliJ to reproduce the failing tests locally. We offer a quick test running howto in the section Final build system checks in our setup guide.

koppor · 2025-02-10T11:52:31Z

+            BibliographyConsistencyCheck.Result result = consistencyCheck.check(entries);
+
+            if (outputFormat.isEmpty()) {
+                Writer ouputStreamWriter = new OutputStreamWriter(System.out);


´new OutputStreamWriteris anAutoclosable`. Wrap it with try-with-resources.

koppor · 2025-02-10T12:01:10Z

+                return;
+            }
+
+            String outputFileName = fileName.get().substring(0, fileName.get().lastIndexOf('.')) + "_consistency_check." + outputFormat.get().toLowerCase();


This needs to be a seperate method.

For ease, I would just call

org.jabref.model.strings.StringUtil#getCorrectFileName(filename, ".consistency-check", outputFormat.get().toLowerCase()

(and accept that . is used instead of _)

This part is not clear.
I have commented the code. Is that correct?

koppor · 2025-02-10T12:03:26Z

+
+            try (Writer writer = new FileWriter(outputFileName, StandardCharsets.UTF_8)) {
+                if ("csv".equalsIgnoreCase(outputFormat.get())) {
+                    BibliographyConsistencyCheckResultCsvWriter csvWriter = new BibliographyConsistencyCheckResultCsvWriter(result, writer, entryTypesManager, databaseContext.getMode());


Better wrap this in try-with-resources; not sure what happens with the writer; if there are troubles, please open the writers both in the try-with-resources (and accept duplication of writer creation)

Writer implements AutoCloseable and is used inside a try-with-resources block, it will be closed automatically. No need to close it manually.

BibliographyConsistencyCheckResultCsvWriter opens a CSVWriter, which uses the Writer.

You close the writer, but not the CSVWriter.

My gut feeling says that the outer writers should be closed - and propagate into the inner writers.

- introduce --procelain - try-with-resources - remove unused variable in test - Fix ! vs . (. should be used)

koppor · 2025-02-10T22:50:41Z

I took the liberty to continue the implementation. Introduced --porcelain

…u16095/jabref into cliForConsistencyResult

koppor · 2025-02-10T23:44:36Z

try-with should not span nearly the whole method; it should very local.

koppor

I think, it is good to go now:

Used inheritance for using BibliographyConsistencyCheckResultCsvWriter at the output
Did not close System.out
Does not output to file - this is the job of the operating system. Read about pipes and filters. Works on both Windows and Linux.
Exceptions always need to be logged. (Only in very rare case one omits them whie logging)
Exclamation mark in Enlglish is for screaming. Therefore, I changed it to dot.

Thankx to your test, I found an issue with closing System.out

priyanshu16095 added 3 commits February 9, 2025 19:57

Add cli for txt and csv writer

f02ec59

Add properties to JabRef_en.properties

03ff6a6

Make outputFormat an optional variable

5fd5f9c

github-actions Bot reviewed Feb 9, 2025

View reviewed changes

Add a CHANGELOG.md entry

29f0825

github-actions Bot reviewed Feb 9, 2025

View reviewed changes

priyanshu16095 added 2 commits February 9, 2025 23:04

Change in CHANGELOG.md entry and remove a unit test

2274449

Remove a temporary change

1597dfb

koppor requested changes Feb 9, 2025

View reviewed changes

Use small letters

b263876

This comment was marked as outdated.

Sign in to view

koppor requested changes Feb 10, 2025

View reviewed changes

Merge branch 'main' into cliForConsistencyResult

272f721

github-actions Bot reviewed Feb 10, 2025

View reviewed changes

Print result to console using OutputStreamWriter

0ad2485

priyanshu16095 commented Feb 10, 2025

View reviewed changes

github-actions Bot reviewed Feb 10, 2025

View reviewed changes

priyanshu16095 added 2 commits February 10, 2025 16:24

Remove repeated CHANGELOG.md entry and pass checks

287b074

Use lower case letters in CHANGELOG.md entry

b575098

koppor requested changes Feb 10, 2025

View reviewed changes

priyanshu16095 and others added 3 commits February 10, 2025 20:56

Wrap writers in try-with-resource block

2c8c4c5

Fix resources

0644e35

Streamline code

eea3769

- introduce --procelain - try-with-resources - remove unused variable in test - Fix ! vs . (. should be used)

koppor added 2 commits February 10, 2025 23:50

Merge branch 'main' into cliForConsistencyResult

d565aac

Fix link in CHANGELOG.md

ccac2ce

koppor marked this pull request as ready for review February 10, 2025 22:55

koppor enabled auto-merge February 10, 2025 22:56

This comment was marked as outdated.

Sign in to view

koppor added 2 commits February 11, 2025 00:02

Fix tests

bb485f8

Merge branch 'cliForConsistencyResult' of https://github.com/priyansh…

d2a3474

…u16095/jabref into cliForConsistencyResult

This comment was marked as outdated.

Sign in to view

koppor mentioned this pull request Feb 10, 2025

IEEE abbreviations not consistent JabRef/abbrv.jabref.org#183

Open

Fix test

c042cb7

This comment was marked as outdated.

Sign in to view

koppor added 2 commits February 11, 2025 00:43

Try auto-flush

0d92130

Streamline code

4f71b2d

koppor approved these changes Feb 10, 2025

View reviewed changes

koppor added this pull request to the merge queue Feb 10, 2025

calixtus approved these changes Feb 10, 2025

View reviewed changes

Merged via the queue into JabRef:main with commit f7b2ac0 Feb 11, 2025

Uh oh!

Conversation

priyanshu16095 commented Feb 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fixes #11984

Mandatory checks

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

priyanshu16095 commented Feb 9, 2025

Uh oh!

koppor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

koppor commented Feb 10, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

koppor commented Feb 10, 2025

Uh oh!

koppor left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

priyanshu16095 commented Feb 9, 2025 •

edited

Loading