Skip to content

Changing handling on all-modules.go and the 'go generate' feature #355

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

Merged

Conversation

MorquinDevlar
Copy link
Contributor

@MorquinDevlar MorquinDevlar commented Apr 29, 2025

Description

When forking the repository, and running "go generate" modules/all-modules.go is created or updated, but since it was previously tracking in the repository it's then added to unstaged files for commit. This is not optimal, as it could potentially mean someone would push their custom all-modules.go file to the main repository.

It's already in .gitignore, but since it was already tracked this is not enough to have ti not appear as a changed file when running "go generate".

Changes

  • Added a default modules/all-modules.go to the repo
  • Removed all-modules.go from .gitignore
  • Removed references to 'go generate' in the Makefile
  • Removed cmd/generate/_all-modules.go
  • Added proper sorting of modules in modules-import.go

@MorquinDevlar MorquinDevlar requested a review from Volte6 as a code owner April 29, 2025 06:03
@Volte6
Copy link
Member

Volte6 commented Apr 29, 2025

Dunno whyni thought this was a good idea. Why don't we just commit the generated file with the current set of modules? Thst makes it simple. Make targets that copy over it need to be updated though

@MorquinDevlar
Copy link
Contributor Author

Would it not invariably change when someone runs "go generate" and then get added to their unstaged files?

@MorquinDevlar
Copy link
Contributor Author

I've had to "re-fork" so many times because I messed up something in my own repository trying to keep it up to date with the upstream one.

Anything we can do to make it simple would be much appreciated :-)

Copy link
Member

@Volte6 Volte6 left a comment

Choose a reason for hiding this comment

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

I think this PR is a good idea overall. We need to go back to how it was, more or less. I think we should include the following:

  • Instead of deleting it, run go generate and commit the results
  • Remove modules/all-modules.go from .gitignore
  • Remove all references to make ungenerate from Makefile
  • Delete cmd/generate/_all-modules.go
  • Add sort.Strings(pkgs) to line 47 in module-imports.go, so that we always get a predictable order of modules (and prevent unnecessary file changes).

This makes go generate an optional "helper" step that can be run only when adding or removing modules.

@MorquinDevlar MorquinDevlar requested a review from Volte6 April 30, 2025 11:44
@MorquinDevlar
Copy link
Contributor Author

Changes are all done now.

Copy link
Member

@Volte6 Volte6 left a comment

Choose a reason for hiding this comment

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

Looks good, we still need the initial all-modules.go to be present (run go generate and commit results). The CI build fails as will anyone trying to run the project without it.

@MorquinDevlar
Copy link
Contributor Author

Looks good, we still need the initial all-modules.go to be present (run go generate and commit results). The CI build fails as will anyone trying to run the project without it.

I actually thought I did. Did I not add it correctly? It was supposed to be this commit: 27c6854

@Volte6
Copy link
Member

Volte6 commented Apr 30, 2025

Looks good, we still need the initial all-modules.go to be present (run go generate and commit results). The CI build fails as will anyone trying to run the project without it.

I actually thought I did. Did I not add it correctly? It was supposed to be this commit: 27c6854

My bad, I misinterpreted what I was seeing.

Copy link
Member

@Volte6 Volte6 left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@MorquinDevlar
Copy link
Contributor Author

I am very happy to help :)

@MorquinDevlar MorquinDevlar changed the title Removing the modules/all-modules.go from the Git index Changing handling on all-modules.go and the 'go generate' feature Apr 30, 2025
@MorquinDevlar MorquinDevlar requested a review from Volte6 April 30, 2025 19:37
@MorquinDevlar
Copy link
Contributor Author

I forgot to add the sorting - that is now added in the last commit.

@MorquinDevlar MorquinDevlar merged commit cbfd4b9 into GoMudEngine:master Apr 30, 2025
2 checks passed
@MorquinDevlar MorquinDevlar deleted the repository-preparation branch April 30, 2025 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants