Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Mod Loader and MC Version changes to diff #48

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

Wxrlds
Copy link
Contributor

@Wxrlds Wxrlds commented Oct 13, 2024

  • Adds sections # Minecraft and # Loaders to the diff output
  • Add header-size option to specify number of # in front of the headings
  • Add test to make sure diff has correct format

@Wxrlds Wxrlds marked this pull request as ready for review October 13, 2024 19:02
@juraj-hrivnak juraj-hrivnak added the enhancement Enhancement of an existing feature label Oct 13, 2024
@juraj-hrivnak juraj-hrivnak self-requested a review November 24, 2024 16:26
Copy link
Owner

@juraj-hrivnak juraj-hrivnak left a comment

Choose a reason for hiding this comment

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

Thank you for the PR. I see very good things like: instead of using mutable collections, you are using map(); also the printing and writing to file side effects are separate from the main function.

The resources for the tests should not be included like this. The best would be to generate them when the tests are set up and then clean them up afterwards. Also instead of using a Python script like this, it should be generated in the setup.

Feel free to ask if anything is not clear and good luck!

src/commonMain/kotlin/teksturepako/pakku/cli/cmd/Diff.kt Outdated Show resolved Hide resolved
src/commonMain/kotlin/teksturepako/pakku/cli/cmd/Diff.kt Outdated Show resolved Hide resolved
src/commonMain/kotlin/teksturepako/pakku/cli/cmd/Diff.kt Outdated Show resolved Hide resolved
updatedProjects.forEach { terminal.info("! $it") }
}

private fun writeDiffChangesToFile(
Copy link
Owner

Choose a reason for hiding this comment

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

This function could be generalized to work for each type of change, if possible. Some patterns repeat, you could probably do it similarly to the above example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see your point, but the biggest problem I see here is that every time we write to a file, we check if there are more changes in the loaders or projects to insert the newline.

And since the different change types (MC version, loader, project) are looking for the next loader and project or just loader or just project, I don't see an easy way to do this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement of an existing feature
Projects
Status: In review🔎
Development

Successfully merging this pull request may close these issues.

2 participants