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

Enable checking git diffs for modifying "frozen" areas #146

Closed
wants to merge 7 commits into from

Conversation

koppor
Copy link
Contributor

@koppor koppor commented Sep 12, 2023

The check feature now contains a check if during an update of the CHANGELOG.md file, areas of released versions are touched.
For instance, a contributor starts a pull request and adds their change to CHANGELOG.md.
Then, the main branch is released.
Now, the PR should move the changelog entry to the new [unreleased] section.
Therefore, heylogs warns in case there are modifications inside blocks of released versions.
One has to enable this check explicitly by the -g command line parameter.

I had to include JGit as dependency. Therefore, the minimum Java requirement is Java 11. I think, this is OK, because Java 17 should be available at most systems.

I use the multiline string feature in the tests, therefore, the tests are Java 17.

Since I wanted to output some debug information, I added tinylog as logging framework. The main reason is that Logger.debug can be used without any instantiation. - The logging traffic of JGit is routed to tinylog.

Example output:

   112:1  error  Change in a released version  releasechangesfrozen
   253:1  error  Change in a released version  releasechangesfrozen
   400:1  error  Change in a released version  releasechangesfrozen
   427:1  error  Change in a released version  releasechangesfrozen
   507:1  error  Change in a released version  releasechangesfrozen
   554:1  error  Change in a released version  releasechangesfrozen
   738:1  error  Change in a released version  releasechangesfrozen
   881:1  error  Change in a released version  releasechangesfrozen
  1125:1  error  Missing ref link              all-h2-contain-a-version
  1000:1  error  Change in a released version  releasechangesfrozen

Due to the way of visiting the Markdown tree, errors in the last block come as very last. I can fix this in a follow-up work, because I think, this issue occurs very seldom - and is just a "UX" issue.

- Upgrade to Java 11 (tests: Java 17)
- Add JGit as dependency
- Add tinylog as logging framework
@koppor
Copy link
Contributor Author

koppor commented Sep 13, 2023

There is a "simpler" way with the keepac CLI tool. Explained at NiclasvanEyk/keepac#20 (comment).

Even though, I very like the Java eco system, I am OK to use a go-based tool + jq to solve my issue.

@charphi
Copy link
Member

charphi commented Sep 13, 2023

There is a "simpler" way with the keepac CLI tool. Explained at NiclasvanEyk/keepac#20 (comment).

I was not aware of this tool. Lots of good ideas in there.

@charphi
Copy link
Member

charphi commented Sep 13, 2023

Regarding the migration to Java11+, I need to do some research first.

My problem is that most of the tools created in this organisation are support tools for a larger project that needs to run on Java8.
There is a new version of this large project that requires Java17, but the old one has not yet reached its end of life.

Fortunately, I already have a workaround in mind, but I need to test it to make sure it works properly.

@charphi charphi added the enhancement New feature or request label Sep 13, 2023
@koppor
Copy link
Contributor Author

koppor commented Oct 22, 2023

Fortunately, I already have a workaround in mind, but I need to test it to make sure it works properly.

Did you find some workaround? If not, maybe, "just" implementing a "toJson()" functionality (as clparse offers) would suffice to support the feature. (clparse -> json -> filter -> dif)

# Conflicts:
#	CHANGELOG.md
#	heylogs-cli/src/main/java/nbbrd/heylogs/cli/CheckCommand.java
@koppor
Copy link
Contributor Author

koppor commented Oct 22, 2023

The CLI maybe needs more thought.

check --gitdiff CHANGELOG.md

Results in

Invalid value for option '--gitdiff': cannot convert 'CHANGELOG.md' to Pair (java.lang.IllegalArgumentException: Invalid commit range format! Expected format is id1...id2.)

I have been in the rabbit whole of Picoli for that. = versus space for sub command beloging. I don't remember excactly now, but I wanted to share this now ^^.

@koppor
Copy link
Contributor Author

koppor commented Oct 22, 2023

The CLI maybe needs more thought.

check CHANGELOG.md --gitdiff 

Works, so maybe, leave as is ^^

(I could not come up with a better word for gitdiff)

@koppor
Copy link
Contributor Author

koppor commented Oct 22, 2023

I could not manage to have errors properly output. Maybe you have an idea?

2023-10-22 20:19:10 [main] internal.heylogs.GitDiffRule.initialize()
ERROR: Could not read from git repository
check C:\git-repositories\jabref\CHANGELOG.md --debug --gitdiff main...non-valid-ref

@charphi
Copy link
Member

charphi commented Nov 10, 2023

I'm still thinking about this gitdiff feature.
I like to keep the project simple and any git feature brings complexity and a few dependencies.
And there is the Java version problem ...
My workaround is cumbersome and would require a lot of work so I prefer not to.

Anyway, I have a few solutions:

@koppor
Copy link
Contributor Author

koppor commented Nov 10, 2023

The latest merge of development branch broke this functionality. Too much effort to maintain. Exporting to JSON is the better way!

@koppor koppor closed this Nov 10, 2023
@koppor
Copy link
Contributor Author

koppor commented Nov 14, 2023

I finally made the switch to clparse (for this use case) at JabRef/jabref#10641. So, no need to think through this use-case further.

(just for information: clparse needed to be changed to support "my" CHANGELOG files ^^ -- marcaddeo/clparse#22)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants