Skip to content

[NOT TICKET] FileSource_fixes #330

Merged
merged 2 commits into from
Sep 10, 2025
Merged

Conversation

Ioannis
Copy link
Contributor

@Ioannis Ioannis commented Sep 1, 2025

No description provided.

@Ioannis Ioannis marked this pull request as draft September 1, 2025 07:41
@Ioannis Ioannis marked this pull request as ready for review September 1, 2025 10:00
@Ioannis Ioannis force-pushed the FileSource_fixes branch 7 times, most recently from 147161d to ace8fb7 Compare September 1, 2025 16:17
Copy link
Contributor

@benno benno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's hard to parse this PR because it seems to be doing multiple things. Can we split apart the FileSource bug fixes from the refactoring of duplicateFilterEntityData? (If we're going to do that refactoring, I'd like to address the overlap with TableMetaTrait and maybe a third similar implementation rather than introduce a new trait that does basically the same thing.)

@Ioannis
Copy link
Contributor Author

Ioannis commented Sep 2, 2025

It's hard to parse this PR because it seems to be doing multiple things. Can we split apart the FileSource bug fixes from the refactoring of duplicateFilterEntityData? (If we're going to do that refactoring, I'd like to address the overlap with TableMetaTrait and maybe a third similar implementation rather than introduce a new trait that does basically the same thing.)

@benno I have already addressed that. The first commit contains only the fixes, while the second commit includes the refactoring

(If we're going to do that refactoring, I'd like to address the overlap with TableMetaTrait and maybe a third similar implementation rather than introduce a new trait that does basically the same thing.)

Thank you for bringing this to my attention. Refactoring this area is next on my list of tasks, and the initial changes I made are intended as a foundation for the broader improvements planned.

However, I would prefer to address the refactoring of in a separate pull request, since it involves functional changes. This will help keep the changes focused and as minimal as possible, specifically targeting the merge between and . duplicateFilterEntityData``duplicateFilterEntityData``filterMetadataFields

With the second commit, my goal is to break the code into smaller, more manageable pieces. The PipelineTable.php file is quite large, and I am not altering its underlying functionality at this stage. Rather, I am working to compartmentalize the code to improve readability and maintainability.

@Ioannis
Copy link
Contributor Author

Ioannis commented Sep 5, 2025

@benno removed the refactoring. This pull request contains only bug fixes.

@@ -207,6 +207,9 @@ msgstr "Name"
msgid "ordr"
msgstr "Order"

msgid "other.value"
Copy link
Contributor Author

@Ioannis Ioannis Sep 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benno Currently this is used by the Email Verifier plugin. Since it is very generic, i think it makes sense to be in the core code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be in its own issue since FileSource doesn't seem to be using it. Once that's done we can merge this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@benno benno merged commit 9dd919d into COmanage:develop Sep 10, 2025
@Ioannis Ioannis deleted the FileSource_fixes branch September 10, 2025 17:23
Sign in to join this conversation on GitHub.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants