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

Custom section name in user readme #110

Merged
merged 1 commit into from
Nov 28, 2022
Merged

Custom section name in user readme #110

merged 1 commit into from
Nov 28, 2022

Conversation

guilyx
Copy link
Contributor

@guilyx guilyx commented Nov 24, 2022

Basically, allow any other section name tag than "waka" in the readme, to avoid conflicts with other projects that may use the same default one. I'll do the same PR for the other one...

Currently testing.

@guilyx guilyx changed the title WIP: Custom section name in user readme Custom section name in user readme Nov 25, 2022
@guilyx
Copy link
Contributor Author

guilyx commented Nov 25, 2022

@athul

@yozachar
Copy link
Collaborator

yozachar commented Nov 26, 2022

@guilyx, thanks for the PR! It's a good idea.

But can you please re-do the PR with just one commit, since the changes are small and perfectly focused for one commit? Also add relevant notes to to the Readme's #tweaks section.

Thanks!


"Currently testing."

If so, mark your PR as draft.

@guilyx
Copy link
Contributor Author

guilyx commented Nov 26, 2022

Hey, actually marked it as WIP and then edited it out - testing is done atm.
Annnnd why don't you just Squash and Merge ? Not an option ?
Will do the readme thing

@yozachar yozachar self-assigned this Nov 26, 2022
@yozachar yozachar added the waiting for response Waiting for user response label Nov 26, 2022
@guilyx
Copy link
Contributor Author

guilyx commented Nov 28, 2022

@joe733 Squashed all the commits in one and updated readme :)

Copy link
Collaborator

@yozachar yozachar left a comment

Choose a reason for hiding this comment

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

Perfect! Thanks for the PR.

Note-to-self: Need input validation for os.getenv("SECTION_NAME")

@yozachar yozachar removed the waiting for response Waiting for user response label Nov 28, 2022
@guilyx guilyx force-pushed the master branch 2 times, most recently from 797f6a3 to 3406025 Compare November 28, 2022 11:28
@guilyx
Copy link
Contributor Author

guilyx commented Nov 28, 2022

Hey again @joe733
Made it cleaner, added an input validation etc

@yozachar
Copy link
Collaborator

yozachar commented Nov 28, 2022

Ah, thank you. But it was a note-to-self. Now it might require more changes. Let me review.

@yozachar yozachar marked this pull request as draft November 28, 2022 11:35
@@ -58,6 +58,7 @@
INPUT_REPOSITORY='<REPOSITORY SLUG>'
INPUT_COMMIT_MESSAGE='<COMMIT MESSAGE>'
INPUT_SHOW_TITLE='True'
INPUT_SECTION_NAME='wakacustom'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Keep, it as waka

-INPUT_SECTION_NAME='wakacustom'
+INPUT_SECTION_NAME='waka'

@@ -336,7 +342,7 @@ def churn(old_readme: str, /) -> str | None:
if not (waka_stats := fetch_stats()):
logger.error('Unable to fetch data, please rerun workflow')
sys.exit(1)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Stray (tab) white space

@yozachar
Copy link
Collaborator

I do want to make changes, but for now let it be.

@yozachar yozachar marked this pull request as ready for review November 28, 2022 11:44
@yozachar yozachar merged commit 72af24c into athul:master Nov 28, 2022
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