`v0.5.0` #102

tuunit · 2023-06-08T21:11:06Z

contains:
feat: add direct urls for comments #98
bugfix: release url generation #100
feat: add pagination support #101

has been tested here:

MAX_LINES: 125
https://github.com/tuunit/github-activity-testing/actions/runs/5215667001
https://github.com/tuunit/github-activity-testing/tree/8aea49f4045b28a9af35e027511de03ca0672b80

MAX_LINES: 5
https://github.com/tuunit/github-activity-testing/tree/108a03b9615ed203fd40090858038dd0cb8bf697
https://github.com/tuunit/github-activity-testing/actions/runs/5215708948

- add direct urls for comments and replace custom url handling with html_url property provided by github api - remove redundant else branches - separate into if blocks for better readability - remove unnecessary constant for github base url Co-authored-by: James George <jamesgeorge998001@gmail.com>

parameter

jamesgeorge007

There're a few edge cases to be considered.

Exiting early if there are no changes to be made.
If there're empty lines after the  comment. This is handled if the  comment is present. If it's not the case and if the line after the  comment is empty, let's show a message conveying the  comment was not found, and it will be inserted towards the end. Also, empty lines were found after the  comment and will be replaced by the activity events.

github-activity-readme/index.js

Lines 188 to 224 in 21d9a86

    
           const oldContent = readmeContent.slice(startIdx + 1, endIdx).join("\n"); 
        
           const newContent = content 
        
             .map((line, idx) => `${idx + 1}. ${line}`) 
        
             .join("\n"); 
        
           if (oldContent.trim() === newContent.trim()) 
        
             tools.exit.success("No changes detected"); 
        
           startIdx++; 
        
           // Recent GitHub Activity content between the comments 
        
           const readmeActivitySection = readmeContent.slice(startIdx, endIdx); 
        
           if (!readmeActivitySection.length) { 
        
             content.some((line, idx) => { 
        
               // User doesn't have 5 public events 
        
               if (!line) { 
        
                 return true; 
        
               } 
        
               readmeContent.splice(startIdx + idx, 0, `${idx + 1}. ${line}`); 
        
             }); 
        
             tools.log.success(`Wrote to ${TARGET_FILE}`); 
        
           } else { 
        
             // It is likely that a newline is inserted after the <!--START_SECTION:activity--> comment (code formatter) 
        
             let count = 0; 
        
             readmeActivitySection.some((line, idx) => { 
        
               // User doesn't have 5 public events 
        
               if (!content[count]) { 
        
                 return true; 
        
               } 
        
               if (line !== "") { 
        
                 readmeContent[startIdx + idx] = `${count + 1}. ${content[count]}`; 
        
                 count++; 
        
               } 
        
             }); 
        
             tools.log.success(`Updated ${TARGET_FILE} with the recent activity`); 
        
           }

jamesgeorge007 · 2023-06-29T09:29:05Z

index.js

+        const resp = await tools.github.activity.listPublicEventsForUser({
+          username: GH_USERNAME,
+          per_page: 100,
+          page: page,


Suggested change

page: page,

page,

i'm personally not a fan of the shortened version in this instance as all the other parameters are explicitly named or set with a fixed value. it looks kind of strange to me when there is a line missing the key value pair. but if you have a strong preference for it i can change it.

I find the object shorthand syntax handy and always prefer it. I see the point here not to mix it up and be explicit. If you ask me, I'd always go for the syntax sugar. If we can use the ES6 semantics, then why not leverage it? Feel free to take the call here.

index.js

jamesgeorge007 · 2023-06-29T09:31:47Z

index.js

-    }
+    // Append new content
+    events.forEach((line, idx) =>
+      readmeContent.splice(startIdx + idx, 0, `${idx + 1}. ${line}`)


Suggested change

readmeContent.splice(startIdx + idx, 0, `${idx + 1}. ${line}`)

readmeContent.splice(startIdx + idx, 1, `${idx + 1}. ${line}`)

There can be an edge case where the  comment got removed, and the further runs will append entries towards the end rather than replacing them.

On second thought, how about replacing the entry altogether instead of the splice approach?

Suggested change

readmeContent.splice(startIdx + idx, 0, `${idx + 1}. ${line}`)

readmeContent[startIdx + idx] = `${idx + 1}. ${line}`

I think you might have misunderstood how the logic works.

If the end section comment has been found all lines between Start and including the end section comment will be removed:
https://github.com/jamesgeorge007/github-activity-readme/blob/rc/0.5.0/index.js#L202-L205

The forEach in line in line 215 - 217 uses the splice method (starting after the start section comment) to insert each single message after each other. We cannot use the array notation here as that might cause an array out of bounds exception or overwrite any following other lines.

After each message was inserted using splice we add back the end section comment:

https://github.com/jamesgeorge007/github-activity-readme/blob/rc/0.5.0/index.js#L220-L225

But you are right, if the end comment got removed by the user by accident and the old messages do still exist. We would append those messages with the new ones and put the new end comment in between. That would cause the issue that we would keep the old message forever. Unfortunately, I don't see any easy solution to deal with that kind of user error. How would we identify what is part of our list and what was added by the user on purpose? Do you have any ideas?

Yeah, let's not handle the case if the  comment got removed later on; was wondering if it would be helpful to log a message in such a case.

If there're empty lines after the comment. This is handled if the comment is present. If it's not the case and if the line after the comment is empty, let's show a message conveying the comment was not found, and it will be inserted towards the end. Also, empty lines were found after the comment and will be replaced by the activity events.

index.js

jamesgeorge007 · 2023-06-29T09:47:31Z

@tuunit, for features/enhancements, let's ensure it is discussed before going ahead with the implementation. This would give us the time to hear from the community as well. Also, it'd be easier to review PRs individually :)

Since this involves a major refactor, shall we make a patch release to get #100 out?

tuunit · 2023-06-29T17:29:34Z

@jamesgeorge007 no problem I can just cherry pick the commit for the fix and create into another branch and create branch. I'll check your other comments later :)

tuunit · 2023-07-03T19:37:09Z

There're a few edge cases to be considered.

Exiting early if there are no changes to be made.

If there're empty lines after the  comment. This is handled if the  comment is present. If it's not the case and if the line after the  comment is empty, let's show a message conveying the  comment was not found, and it will be inserted towards the end. Also, empty lines were found after the  comment and will be replaced by the activity events.

Regarding the edge cases:

The new implementation always replaces everything between the start and end section comment and tries to commit and push it. In my opinion, we should not try to do any kind of "change detection". Git is the sophisticated change detection tool of choice and therefore should deal with this. No new commit or push will be happening if there wasn't any change. Additionally, this eliminates any logic necessary to deal with changes to the MAX_LINES and other parameters as we will always have a clean slate by removing all the previous messages.
As for empty lines between the start and end section comment, this will be dealt with by the new logic as well as they will always be deleted. For empty lines after the start comment without an end comment. They might be intentionally by the user and therefore will be kept with the new implementation.

tuunit · 2023-07-03T19:37:36Z

@jamesgeorge007 I hope my explanations help to understand the new logic. I know it is a "major" change but I hope it leads to more stability 😊

jamesgeorge007 · 2023-07-09T17:02:09Z

The new implementation always replaces everything between the start and end section comment and tries to commit and push it. In my opinion, we should not try to do any kind of "change detection". Git is the sophisticated change detection tool of choice and therefore should deal with this. No new commit or push will be happening if there wasn't any change. Additionally, this eliminates any logic necessary to deal with changes to the MAX_LINES and other parameters as we will always have a clean slate by removing all the previous messages.

I concur. But, we used to exit early and log an appropriate message previously. Isn't it better to keep the behavior? We could even leverage Git to check for changes made during the workflow run instead of comparing the contents manually.

As for empty lines between the start and end section comment, this will be dealt with by the new logic as well as they will always be deleted. For empty lines after the start comment without an end comment. They might be intentionally by the user and therefore will be kept with the new implementation.

#102 (comment)

tuunit and others added 3 commits June 6, 2023 09:45

bugfix: release url generation

e74c153

feat: add pagination support and fix a bug when increasing the max_lines

9d9c8bf

parameter

tuunit requested a review from jamesgeorge007 June 8, 2023 21:12

jamesgeorge007 requested changes Jun 29, 2023

View reviewed changes

jamesgeorge007 changed the title ~~build: rc/0.5.0~~ v0.5.0 Jun 29, 2023

change error message handling and remove uncessary debug log entry

f5c20aa

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`v0.5.0` #102

`v0.5.0` #102

tuunit commented Jun 8, 2023 •

edited

jamesgeorge007 left a comment •

edited

jamesgeorge007 Jun 29, 2023

tuunit Jul 3, 2023

jamesgeorge007 Jul 9, 2023

jamesgeorge007 Jun 29, 2023

jamesgeorge007 Jun 29, 2023

tuunit Jul 3, 2023

tuunit Jul 3, 2023

jamesgeorge007 Jul 9, 2023

jamesgeorge007 commented Jun 29, 2023 •

edited

tuunit commented Jun 29, 2023 •

edited

tuunit commented Jul 3, 2023

tuunit commented Jul 3, 2023 •

edited

jamesgeorge007 commented Jul 9, 2023

	const oldContent = readmeContent.slice(startIdx + 1, endIdx).join("\n");
	const newContent = content
	.map((line, idx) => `${idx + 1}. ${line}`)
	.join("\n");

	if (oldContent.trim() === newContent.trim())
	tools.exit.success("No changes detected");

	startIdx++;

	// Recent GitHub Activity content between the comments
	const readmeActivitySection = readmeContent.slice(startIdx, endIdx);
	if (!readmeActivitySection.length) {
	content.some((line, idx) => {
	// User doesn't have 5 public events
	if (!line) {
	return true;
	}
	readmeContent.splice(startIdx + idx, 0, `${idx + 1}. ${line}`);
	});
	tools.log.success(`Wrote to ${TARGET_FILE}`);
	} else {
	// It is likely that a newline is inserted after the <!--START_SECTION:activity--> comment (code formatter)
	let count = 0;

	readmeActivitySection.some((line, idx) => {
	// User doesn't have 5 public events
	if (!content[count]) {
	return true;
	}
	if (line !== "") {
	readmeContent[startIdx + idx] = `${count + 1}. ${content[count]}`;
	count++;
	}
	});
	tools.log.success(`Updated ${TARGET_FILE} with the recent activity`);
	}

	readmeContent.splice(startIdx + idx, 0, `${idx + 1}. ${line}`)
	readmeContent.splice(startIdx + idx, 1, `${idx + 1}. ${line}`)

	readmeContent.splice(startIdx + idx, 0, `${idx + 1}. ${line}`)
	readmeContent[startIdx + idx] = `${idx + 1}. ${line}`

v0.5.0 #102

Are you sure you want to change the base?

v0.5.0 #102

Conversation

tuunit commented Jun 8, 2023 • edited

jamesgeorge007 left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesgeorge007 commented Jun 29, 2023 • edited

tuunit commented Jun 29, 2023 • edited

tuunit commented Jul 3, 2023

tuunit commented Jul 3, 2023 • edited

jamesgeorge007 commented Jul 9, 2023

`v0.5.0` #102

`v0.5.0` #102

tuunit commented Jun 8, 2023 •

edited

jamesgeorge007 left a comment •

edited

jamesgeorge007 commented Jun 29, 2023 •

edited

tuunit commented Jun 29, 2023 •

edited

tuunit commented Jul 3, 2023 •

edited