Skip to content

feat: Make merge <input> argument optional when --from-folder is given#191

Merged
italvi merged 9 commits intomainfrom
fix-merge
Jul 3, 2024
Merged

feat: Make merge <input> argument optional when --from-folder is given#191
italvi merged 9 commits intomainfrom
fix-merge

Conversation

@mmarseu
Copy link
Copy Markdown
Collaborator

@mmarseu mmarseu commented May 27, 2024

Overhauls the loading of input files in invoke_merge().

  • The user no longer needs to provide direct inputs as positional arguments if --from-folder is given and the folder contains at least two input files.
  • Input files in the folder are now sorted in the same way the operating system sorts files.
  • Inputs contained in --from-folder that are also specified as positional arguments are no longer merged twice.

This is a breaking change for two reasons:

  • If the input folder is empty, the program used to exit. It now continues with a warning, if the total number of inputs is 2 or higher.
  • The console output has changed slightly. Paths loaded from the input folder are now printed as debug log entries. Any scripts which relied on the paths being printed as before must be changed.

Tests for the new behavior will be added to #157.

@mmarseu mmarseu requested a review from italvi May 27, 2024 11:36
@mmarseu mmarseu self-assigned this May 27, 2024
@github-actions github-actions bot added the enhancement New feature or request label May 27, 2024
@mmarseu mmarseu marked this pull request as ready for review May 27, 2024 12:04
@mmarseu mmarseu mentioned this pull request May 28, 2024
5 tasks
@italvi italvi requested a review from CBeck-96 May 31, 2024 13:12
CBeck-96
CBeck-96 previously approved these changes Jun 4, 2024
@mmarseu mmarseu deleted the branch main June 7, 2024 12:30
@mmarseu mmarseu closed this Jun 7, 2024
@mmarseu mmarseu reopened this Jun 7, 2024
@mmarseu mmarseu changed the base branch from usage-error to main June 7, 2024 12:39
@mmarseu mmarseu dismissed CBeck-96’s stale review June 7, 2024 12:39

The base branch was changed.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jun 7, 2024

Coverage

Coverage Report •
FileStmtsMissCoverMissing
__main__.py3192093%217–218, 235, 245, 664–665, 669–674, 676, 679, 689–692, 696, 863
TOTAL16739194% 

Tests Skipped Failures Errors Time
295 2 💤 0 ❌ 0 🔥 5.075s ⏱️

@italvi italvi requested a review from CBeck-96 June 17, 2024 23:59
Copy link
Copy Markdown
Collaborator

@CBeck-96 CBeck-96 left a comment

Choose a reason for hiding this comment

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

Should the documentation be updated?
It can be kept as is, since the information is not wrong, but the case of several input files is not covered.
This might be worth mentioning, since using the input function, the merging order can be influenced (input is merged in order before the folder which contents are merged alphabetically)

@mmarseu
Copy link
Copy Markdown
Collaborator Author

mmarseu commented Jun 18, 2024

Should the documentation be updated? It can be kept as is, since the information is not wrong, but the case of several input files is not covered. This might be worth mentioning, since using the input function, the merging order can be influenced (input is merged in order before the folder which contents are merged alphabetically)

Valid point. The documentation should be improved.

That made me think: Is there any point to requiring an input argument when --from-folder is given? Since merge order is often important, the user has to make sure to name the files in the folder correctly anyways. Then why do we force them to single out the "primary" merge input, if it could simply be the first file in the folder in alphabetical order?

@github-actions github-actions bot added documentation Improvements or additions to documentation settings_changes labels Jun 18, 2024
@CBeck-96
Copy link
Copy Markdown
Collaborator

Should the documentation be updated? It can be kept as is, since the information is not wrong, but the case of several input files is not covered. This might be worth mentioning, since using the input function, the merging order can be influenced (input is merged in order before the folder which contents are merged alphabetically)

Valid point. The documentation should be improved.

That made me think: Is there any point to requiring an input argument when --from-folder is given? Since merge order is often important, the user has to make sure to name the files in the folder correctly anyways. Then why do we force them to single out the "primary" merge input, if it could simply be the first file in the folder in alphabetical order?

Good point. Now that you mention it, it would be more natural to justb (want to) merge a folders contents.

@mmarseu
Copy link
Copy Markdown
Collaborator Author

mmarseu commented Jun 20, 2024

Should the documentation be updated? It can be kept as is, since the information is not wrong, but the case of several input files is not covered. This might be worth mentioning, since using the input function, the merging order can be influenced (input is merged in order before the folder which contents are merged alphabetically)

Valid point. The documentation should be improved.
That made me think: Is there any point to requiring an input argument when --from-folder is given? Since merge order is often important, the user has to make sure to name the files in the folder correctly anyways. Then why do we force them to single out the "primary" merge input, if it could simply be the first file in the folder in alphabetical order?

Good point. Now that you mention it, it would be more natural to justb (want to) merge a folders contents.

Done. Tests will be added to #157

@mmarseu mmarseu changed the title fix: Edge case with multiple inputs and --from-folder in merge feat: Make merge <input> argument optional when --from-folder is given Jun 20, 2024
@italvi italvi requested a review from CBeck-96 June 26, 2024 12:37
@italvi italvi merged commit c9f4d91 into main Jul 3, 2024
@italvi italvi deleted the fix-merge branch July 3, 2024 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation enhancement New feature or request settings_changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants