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

FEAT: Add configuration files #55

Merged
merged 23 commits into from
Aug 22, 2023
Merged

Conversation

oesteban
Copy link
Contributor

Permit git-changelog be configured permanently via config files.

Resolves: #54.

@oesteban oesteban force-pushed the feat/config-files branch 3 times, most recently from 5e09c6a to ba63ef8 Compare August 18, 2023 10:18
@oesteban
Copy link
Contributor Author

Okay, this could count as an MVP.

Unfortunately, I'm not sure I have the bandwidth to write some tests right now.

@pawamoy
Copy link
Owner

pawamoy commented Aug 18, 2023

Thanks, I'll try to review it soon. I'll see if I can also add some tests 👍

Copy link
Owner

@pawamoy pawamoy left a comment

Choose a reason for hiding this comment

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

Thanks a lot! Some things to change. If you don't have the capacity right now, I'll handle it when I get some time, no worries 👍

docs/usage.md Outdated Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
src/git_changelog/cli.py Outdated Show resolved Hide resolved
src/git_changelog/cli.py Outdated Show resolved Hide resolved
src/git_changelog/cli.py Outdated Show resolved Hide resolved
src/git_changelog/cli.py Outdated Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
src/git_changelog/cli.py Show resolved Hide resolved
@oesteban oesteban force-pushed the feat/config-files branch 7 times, most recently from 41964c2 to 5af203f Compare August 21, 2023 10:10
src/git_changelog/cli.py Outdated Show resolved Hide resolved
oesteban added a commit to oesteban/git-changelog that referenced this pull request Aug 21, 2023
This commit addresses
[this comment (pawamoy#55)](pawamoy#55 (comment)).

Please note that this will disallow unsetting options if a hierarchy of
config files were to be implemented at some point.
oesteban added a commit to oesteban/git-changelog that referenced this pull request Aug 21, 2023
This commit addresses
[this comment (pawamoy#55)](pawamoy#55 (comment)).

Please note that this will disallow unsetting options if a hierarchy of
config files were to be implemented at some point.
src/git_changelog/cli.py Outdated Show resolved Hide resolved
@oesteban
Copy link
Contributor Author

Okay, I think this is getting there. However, I've hit some stomper with tests/test_cli.py::test_main. It passes locally on my workstation but doesn't on GHA.

I believe this test is actually just checking that the input is parsed and default settings are set, so I would just remove it (I don't think it covers anymore lines than the other tests).

The problem with the test is that it is checking two different things - whether the settings are properly parsed and then the call to main.

If the goal is to see that the parser is working, I suggest just testing parse_settings instead.

LMKWYT

Copy link
Owner

@pawamoy pawamoy left a comment

Choose a reason for hiding this comment

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

Thanks for following through, and for implementing things with care 🙂

LGTM, just a few suggestion (minor changes).

docs/usage.md Outdated Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
docs/usage.md Outdated Show resolved Hide resolved
src/git_changelog/cli.py Outdated Show resolved Hide resolved
src/git_changelog/cli.py Outdated Show resolved Hide resolved
tests/test_cli.py Outdated Show resolved Hide resolved
@pawamoy
Copy link
Owner

pawamoy commented Aug 21, 2023

It passes locally on my workstation but doesn't on GHA.

I'll take a look.

@pawamoy
Copy link
Owner

pawamoy commented Aug 21, 2023

Actually the test failures I see in CI are weird. If you look at the second and third last runs, there are 2 or 3 tests failing, all because of the git config --get remote.origin.url subprocess. In the last run though, only one test fails on this. And this is completely unrelated to this PR. Let me re-run CI to see if it happens consistently.

@pawamoy
Copy link
Owner

pawamoy commented Aug 21, 2023

OK, better. However we're hitting this: FutureWarning: bump-latest = false option found in config file (/tmp/pytest-of-runner/pytest-0/popen-gw0/test_settings_warning_False_0/.git-changelog.toml).

I think there's a race condition in the tests. We shouldn't create test files in the repository folder, but rather use a tmp directory, move there and only then test things. EDIT: oh but you're already doing that. Not sure what's happening. Will continue to investigate. EDIT 2: oh of course, os.chdir() affects the whole process, not just the current test.

@pawamoy
Copy link
Owner

pawamoy commented Aug 21, 2023

I've fixed the tests, applied Black formatting, and finished fixing Mypy warnings. I'm ready to squash and merge, let me know if there's anything else you'd like to do before that. And thanks again for your amazing work 🚀

@oesteban
Copy link
Contributor Author

oesteban commented Aug 21, 2023 via email

@pawamoy
Copy link
Owner

pawamoy commented Aug 22, 2023

Good catch on the regular expressions written in TOML!

@pawamoy pawamoy merged commit b527ccf into pawamoy:main Aug 22, 2023
16 checks passed
@oesteban oesteban deleted the feat/config-files branch August 22, 2023 09:07
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.

Config from pyproject.toml or .git-changelogrc
3 participants