Support RFC links in Quick Import functionality (#11949) #12043

AU0430 · 2024-10-20T15:17:17Z

Purpose

See #11949
This PR addresses Issue #11949, which requested support for importing RFC using url links in the quick import feature of JabRef.

Changes Made

Enhanced CompositeIdFetcher to Recognize Rfc URL Links:
- Updated the CompositeIdFetcher class to support RFC document URLs.
- Added RfcId.java to parse RFC URLs (e.g., https://www.rfc-editor.org/rfc/rfc7276.html) and extract the RFC ID correctly, even if the URL contains a section fragment (e.g., #section-2.2.2).
Updated Identifier Handling:
- Added RFC-specific parsing logic directly in the CompositeIdFetcher to detect URLs containing "rfc" and extract relevant RFC identifiers.
- This change allows the performSearchById method to correctly identify and handle RFC URLs without needing a separate importer.

How the Issue is Solved

Problem: Users were unable to import RFC documents directly using their full URLs, especially when these URLs included section references.
Solution: Modified the CompositeIdFetcher class to include logic that can extract RFC identifiers from URLs and proceed with the import. This allows users to directly paste RFC URLs into the quick import field, and JabRef will successfully parse and import the relevant RFC entry.

Notes

This PR only supports URLs from https://www.rfc-editor.org. If users encounter other RFC sources, further enhancement might be needed.
Users can now easily use the quick import feature with full RFC URLs, which will enhance the usability of JabRef for RFC references.

Please review these changes, and let me know if there are any adjustments or additional enhancements required. Feedback is highly appreciated to ensure the solution meets user expectations effectively.

koppor · 2024-10-20T15:20:13Z

src/main/java/org/jabref/model/entry/identifier/RfcId.java

+        return Optional.empty();
+    }
+
+    public boolean isValid() {


Why only plain and not with URL?

Add JavaDoc comment!

Hi, maybe I understand something wrong, so I don't need to match plain in this part, but only need to match URLs in this format("https://www.rfc-editor.org/rfc/" + rfc7276.html#section-2.2.2)?

Can the modified part meet your requirements?

Hi, maybe I understand something wrong, so I don't need to match plain in this part, but only need to match URLs in this format("https://www.rfc-editor.org/rfc/" + rfc7276.html#section-2.2.2)?

You do nlt match the URL at all there. therefore I wondered.

Because I was worried that there might be other formats of URLs, I considered just matching the format of rfc****.
My mistake. I am really sorry for causing you trouble.

The method is nowhere used. REmove.

If you would implement it correctly, you need to have to check both the URL and just the ID.

koppor · 2024-10-20T15:22:46Z

src/main/java/org/jabref/model/entry/identifier/RfcId.java

+        }
+    }
+
+    @Override


I think, neither equals nor hashCode need to be overriden.

Just remove that code.

There is also a logic error, because Id is stored lower case and the equals also converts loeer case (unnecessarily)

Thank you for pointing this out! I have removed the overridden equals and hashCode methods as suggested. Please let me know if there's anything else you'd like me to adjust.

github-actions

Your code currently does not meet JabRef's code guidelines.
We use Checkstyle to identify issues.
The tool reviewdog already placed comments on GitHub to indicate the places. See the tab "Files" in you PR.
Please carefully follow the setup guide for the codestyle.
Afterwards, please run checkstyle locally and fix the issues.

You can check review dog's comments at the tab "Files changed" of your pull request.

Add JavaDoc comments Remove unnecessary lowercase

github-actions

Your code currently does not meet JabRef's code guidelines.
We use Checkstyle to identify issues.
The tool reviewdog already placed comments on GitHub to indicate the places. See the tab "Files" in you PR.
Please carefully follow the setup guide for the codestyle.
Afterwards, please run checkstyle locally and fix the issues.

You can check review dog's comments at the tab "Files changed" of your pull request.

AU0430 · 2024-10-20T23:22:06Z

Hello, I have modified my code style, I think there should be no problem, can you test my PR again?

- Removed RFC entry from identifier list(StandardField.java) - Modified RfcId class to extend EprintIdentifier

…link' into Issue11949-quick-import-for-RFC-link

AU0430 · 2024-10-21T00:43:08Z

According to CI test feedback:

I changed the code style according to the checkstyle.xml.
Removed the entry RFC("rfc", "RFC", FieldProperty.IDENTIFIER) from the codebase, thereby reducing the total number of entries from 126 to 125.
Modified the RfcId class to extend EprintIdentifier, enhancing its functionality to integrate better with existing identifier logic.

koppor

Tests for RfcId class missing.

Both rfc7276 and URLs ´https://www.rfc-editor.org/rfc/rfc7276.html` need to be supported

koppor · 2024-10-21T12:08:41Z

src/main/java/org/jabref/model/entry/identifier/RfcId.java

+ * Represents an RFC identifier, which can be used to fetch bibliographic information about the RFC document.
+ * This class supports both plain RFC IDs (e.g., "rfc7276") and full URLs (e.g., "https://www.rfc-editor.org/rfc/" + rfc****.html).
+ */
+public class RfcId extends EprintIdentifier {


Name this class RFC - to be consistent with the other classes in that package.

Thank you for the feedback. I have renamed the class to RFC for consistency with the other classes in the package. The class now follows the naming conventions as requested.

I have removed the unused method(isValid) as suggested.

I have implemented additional test cases in RFCTest.java to verify that the new RFC class handles both plain RFC IDs (e.g., rfc7276) and URLs (e.g., https://www.rfc-editor.org/rfc/rfc7276.html).

koppor · 2024-10-21T12:16:09Z

src/main/java/org/jabref/model/entry/identifier/RfcId.java

+        return Optional.empty();
+    }
+
+    public boolean isValid() {


The method is nowhere used. REmove.

If you would implement it correctly, you need to have to check both the URL and just the ID.

- Replaced RfcId name with RFC class for consistency and clarity. - Updated RFC parsing logic to support both plain RFC IDs (e.g., "rfc7276") and full URLs (e.g., "https://www.rfc-editor.org/rfc/rfc7276.html"). - Added tests in RFCTest to validate proper parsing of RFC identifiers from both formats. - Removed unnecessary method (isValid) improved code clarity. - Adjusted the CompositeIdFetcher to integrate the new RFC class functionalities.

koppor

Some small comments on the test.

koppor · 2024-10-21T18:18:44Z

src/test/java/org/jabref/model/entry/identifier/RFCTest.java

+class RFCTest {
+
+    @Test
+    void testParsePlainRfcId() {


Convert to @ParameterizedTest.You can use @CsvSource. Left: excpected, right: input.

Use assertEquals(expected, new RFC(input).getNormlized().

Also the next test case.

Converted testParsePlainRfcId to a @ParameterizedTest using @CsvSource(done)

Updated testGetExternalUri to verify the URI using Optional.of() for consistent checks.

koppor · 2024-10-21T18:19:11Z

src/test/java/org/jabref/model/entry/identifier/RFCTest.java

+    @Test
+    void testInvalidRfc() {
+        Optional<RFC> rfc = RFC.parse("invalidRfc");
+        assertFalse(rfc.isPresent());


Suggested change

assertFalse(rfc.isPresent());

assertEquals(Optional.empty, rfc);

If something is wrong, the diff can be investigated.

Adjusted testInvalidRfc to use assertEquals(Optional.empty(), rfc).(done)

koppor · 2024-10-21T18:19:56Z

src/test/java/org/jabref/model/entry/identifier/RFCTest.java

+        Optional<URI> uri = rfc.getExternalURI();
+        assertTrue(uri.isPresent());
+        assertEquals("https://www.rfc-editor.org/rfc/rfc7276", uri.get().toString());


Suggested change

Optional<URI> uri = rfc.getExternalURI();

assertTrue(uri.isPresent());

assertEquals("https://www.rfc-editor.org/rfc/rfc7276", uri.get().toString());

assertEquals(Optional.of("https://www.rfc-editor.org/rfc/rfc7276"), rfc.getExternalURI());

Updated testGetExternalUri to verify the URI using Optional.of() for consistent checks.(done)

- Converted `testParsePlainRfcId` to a `@ParameterizedTest` using `@CsvSource`. - Adjusted `testInvalidRfc` to use `assertEquals(Optional.empty(), rfc)`. - Updated `testGetExternalUri` to verify the URI using `Optional.of()` for consistent checks.

github-actions

Your code currently does not meet JabRef's code guidelines.
We use Checkstyle to identify issues.
The tool reviewdog already placed comments on GitHub to indicate the places. See the tab "Files" in you PR.
Please carefully follow the setup guide for the codestyle.
Afterwards, please run checkstyle locally and fix the issues.

You can check review dog's comments at the tab "Files changed" of your pull request.

koppor · 2024-10-23T18:19:35Z

src/test/java/org/jabref/model/entry/identifier/RFCTest.java

+            "rfc7276, rfc7276",
+            "https://www.rfc-editor.org/rfc/rfc7276.html, rfc7276"
+    })
+    void testParseValidRfcIdAndUrl(String input, String expected) {


Please be consistent.

assertEquals first gets expected, thus please, first expected then input

I fix it for myself. Too much context switches.

…rt-for-RFC-link

koppor

OK for me now.

…link' into Issue11949-quick-import-for-RFC-link

koppor · 2024-10-23T18:39:42Z

@ChengPuPu Thank you for the follow up. I was about to open the PR in my IDE and fix it for myself.

koppor · 2024-10-23T18:40:08Z

BTW: No need to disable automerge. Automerge happens if the required workflows run successfully.

AU0430 · 2024-10-23T18:42:45Z

@koppor Thank you very much for your patience and guidance. I have learned a lot about code management through this experience. Thank you very much.

Add RfcId to handle RFC url link to support quick import

1672342

koppor requested changes Oct 20, 2024

View reviewed changes

github-actions bot reviewed Oct 20, 2024

View reviewed changes

ChengPuPu added 2 commits October 21, 2024 03:18

Refactor RFC URL parsing

365e50a

Add JavaDoc comments Remove unnecessary lowercase

Refactor RFC URL parsing

f24ec13

Add JavaDoc comments Remove unnecessary lowercase

github-actions bot reviewed Oct 20, 2024

View reviewed changes

Adjust code style

401f8ce

ChengPuPu and others added 4 commits October 21, 2024 10:50

Adjust code style

ddadb92

Merge branch 'main' into Issue11949-quick-import-for-RFC-link

787dc35

According to CI test feedback:

11a57f1

- Removed RFC entry from identifier list(StandardField.java) - Modified RfcId class to extend EprintIdentifier

Merge remote-tracking branch 'origin/Issue11949-quick-import-for-RFC-…

0649019

…link' into Issue11949-quick-import-for-RFC-link

AU0430 requested a review from koppor October 21, 2024 09:09

koppor requested changes Oct 21, 2024

View reviewed changes

AU0430 requested a review from koppor October 21, 2024 13:14

koppor mentioned this pull request Oct 21, 2024

Identifier: Rename "getNormalized" to "asString" #12054

Merged

7 tasks

koppor requested changes Oct 21, 2024

View reviewed changes

AU0430 requested a review from koppor October 21, 2024 19:51

Merge branch 'main' into Issue11949-quick-import-for-RFC-link

74ed902

Siedlerchr previously approved these changes Oct 22, 2024

View reviewed changes

github-actions bot reviewed Oct 22, 2024

View reviewed changes

adjust code format according to CI test(checkstyle)

d7300a4

AU0430 dismissed Siedlerchr’s stale review via d7300a4 October 22, 2024 20:10

AU0430 requested a review from Siedlerchr October 22, 2024 20:13

Merge branch 'main' into Issue11949-quick-import-for-RFC-link

846e64b

koppor reviewed Oct 23, 2024

View reviewed changes

Merge remote-tracking branch 'origin/main' into Issue11949-quick-impo…

90d769c

…rt-for-RFC-link

koppor added 3 commits October 23, 2024 20:21

Merge remote-tracking branch 'origin/main' into Issue11949-quick-impo…

03f7ea0

…rt-for-RFC-link

Swap expected and input

e0abc10

Fix OpenRewrite issues

29a3e5e

koppor previously approved these changes Oct 23, 2024

View reviewed changes

koppor enabled auto-merge October 23, 2024 18:28

AU0430 disabled auto-merge October 23, 2024 18:37

ChengPuPu added 2 commits October 24, 2024 05:38

fix code style according to checkstyle

891e1f5

Merge remote-tracking branch 'origin/Issue11949-quick-import-for-RFC-…

98e4414

…link' into Issue11949-quick-import-for-RFC-link

AU0430 dismissed koppor’s stale review via 98e4414 October 23, 2024 18:38

koppor approved these changes Oct 23, 2024

View reviewed changes

koppor enabled auto-merge October 23, 2024 18:40

koppor added this pull request to the merge queue Oct 23, 2024

Merged via the queue into JabRef:main with commit b90ef75 Oct 23, 2024
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support RFC links in Quick Import functionality (#11949) #12043

Support RFC links in Quick Import functionality (#11949) #12043

AU0430 commented Oct 20, 2024

koppor Oct 20, 2024

AU0430 Oct 20, 2024

AU0430 Oct 20, 2024

koppor Oct 20, 2024

AU0430 Oct 20, 2024

koppor Oct 21, 2024

koppor Oct 20, 2024

AU0430 Oct 20, 2024

github-actions bot left a comment

github-actions bot left a comment

AU0430 commented Oct 20, 2024

AU0430 commented Oct 21, 2024

koppor left a comment

koppor Oct 21, 2024

AU0430 Oct 21, 2024 •

edited

Loading

koppor Oct 21, 2024

koppor left a comment

koppor Oct 21, 2024

AU0430 Oct 21, 2024

koppor Oct 21, 2024

AU0430 Oct 21, 2024

koppor Oct 21, 2024

AU0430 Oct 21, 2024

github-actions bot left a comment

koppor Oct 23, 2024

koppor left a comment

koppor commented Oct 23, 2024

koppor commented Oct 23, 2024

AU0430 commented Oct 23, 2024

	assertFalse(rfc.isPresent());
	assertEquals(Optional.empty, rfc);

Support RFC links in Quick Import functionality (#11949) #12043

Support RFC links in Quick Import functionality (#11949) #12043

Conversation

AU0430 commented Oct 20, 2024

Purpose

Changes Made

How the Issue is Solved

Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

AU0430 commented Oct 20, 2024

AU0430 commented Oct 21, 2024

koppor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AU0430 Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koppor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koppor left a comment

Choose a reason for hiding this comment

koppor commented Oct 23, 2024

koppor commented Oct 23, 2024

AU0430 commented Oct 23, 2024

AU0430 Oct 21, 2024 •

edited

Loading