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

Implement handling RFCs #1212

Closed
wants to merge 28 commits into from
Closed

Conversation

xno-miner
Copy link
Contributor

@xno-miner xno-miner commented Jun 17, 2024

No description provided.

helpers/github.ts Outdated Show resolved Hide resolved
Co-authored-by: アレクサンダー.eth <[email protected]>
@xno-miner
Copy link
Contributor Author

seems like the tests are using handleDevPoolIssue() without the new parameter. Since the tests are made for non-RFCs they should just pass false everywhere. I'll add that in a sec

@xno-miner
Copy link
Contributor Author

To avoid more potential issues, i changed back all function parameters to original and moved the creation of isRFC and hasCorrectPriceLabels to applyStateChanges()

@0x4007 0x4007 requested a review from rndquu June 17, 2024 14:28
@rndquu
Copy link
Member

rndquu commented Jun 17, 2024

@xno-miner What issue this PR solves? I remember there was a discussion but can't find a related github issue.

@xno-miner
Copy link
Contributor Author

@xno-miner What issue this PR solves? I remember there was a discussion but can't find a related github issue.

ubiquity/work.ubq.fi#54

@0x4007
Copy link
Member

0x4007 commented Jun 17, 2024

@xno-miner What issue this PR solves? I remember there was a discussion but can't find a related github issue.

ubiquity/work.ubq.fi#54

Your pull request body should be:

Resolves ubiquity/work.ubq.fi#54

Read more at https://dao.ubq.fi/devpool-flow#28b52b32bc434aac8d8e6de501ee75e9

@xno-miner xno-miner marked this pull request as draft June 17, 2024 16:20
helpers/github.ts Outdated Show resolved Hide resolved
helpers/github.ts Outdated Show resolved Hide resolved
index.ts Outdated Show resolved Hide resolved
@rndquu
Copy link
Member

rndquu commented Jun 28, 2024

@xno-miner Hey, sorry for a long review, pls take a look at the review comments

@rndquu
Copy link
Member

rndquu commented Jun 29, 2024

@xno-miner Combining both devpool and RFC issue logic complicates the code because of newly introduced if statements (one, two, three).

Perhaps it's more cleaner to:

  1. Create a new branch like ubiquity/devpool-directory#rfc
  2. Refactor the logic to use only ubiquity/devpool-rfc (this eliminates unnecessary if statements for distinguishing devpool and RFC issues)
  3. Create a new repo ubiquity/devpool-rfc and run ubiquity/devpool-directory#rfc there

Right now this is really hard to read.

@xno-miner
Copy link
Contributor Author

@xno-miner Combining both devpool and RFC issue logic complicates the code because of newly introduced if statements (one, two, three).

Perhaps it's more cleaner to:

1. Create a new branch like `ubiquity/devpool-directory#rfc`

2. Refactor the logic to use only `ubiquity/devpool-rfc` (this eliminates unnecessary `if` statements for distinguishing devpool and RFC issues)

3. Create a new repo `ubiquity/devpool-rfc` and [run](https://github.com/ubiquity/devpool-directory/blob/797536f9bfbb6501756b128bcc4251e1d2e3e061/.github/workflows/sync-issues.yml) `ubiquity/devpool-directory#rfc` there

Right now this is really hard to read.

I think a few if statements are cleaner than having two versions of the entire code with only minor differences. I will make a few changes to improve the code readability though.

@xno-miner
Copy link
Contributor Author

I removed the inverse logic involving hasNoPriceLabels and i think the code is much cleaner now. I also removed the devpoolRFCIssues variable and just made devpoolIssues contain issues from both repositories.

@rndquu rndquu self-requested a review July 2, 2024 11:17
@rndquu
Copy link
Member

rndquu commented Jul 2, 2024

@xno-miner I ran this script 2 times and immediately got rate limited by github API. As far as I understand it happens because now we get issues from 2 repositories here and overall perform 2x more API requests to github API.

Perhaps it does make sense to:

  1. Move RFC parsing logic to a separate branch
  2. Move DEVPOOL_OWNER_NAME and DEVPOOL_REPO_NAME to env variables so that we could use them in a fork
  3. Create a new github app (like ubiquibot-rfc) so that we could use its token here
  4. Run ubiquity/devpool-rfc against this newly created branch

This way we use 2 github apps (1st for ubiquity/devpool-directory, 2nd for ubiquity/devpool-rfc) hence reduce chances for being rate limited.

@xno-miner
Copy link
Contributor Author

xno-miner commented Jul 2, 2024

I added the env variables. For devpool their values should be:

DEVPOOL_OWNER_NAME=ubiquity
DEVPOOL_REPO_NAME=devpool-directory
RFC=0

and for RFC they should be:

DEVPOOL_OWNER_NAME=ubiquity
DEVPOOL_REPO_NAME=devpool-rfc
RFC=1

@xno-miner
Copy link
Contributor Author

What's the progress of the review?

@0x4007 0x4007 requested review from rndquu and removed request for rndquu July 22, 2024 03:58
@rndquu rndquu marked this pull request as ready for review July 25, 2024 07:35
Copy link
Member

@rndquu rndquu left a comment

Choose a reason for hiding this comment

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

@xno-miner

It seems the code hasn't been run because process.env.RFC is always evaluating to true (here, here, here, etc...). Env variables are always a string so it doesn't matter if we set process.env.RFC to 0 or 1 because those are not empty string always evaluated to true.

Pls:

  1. Fix process.env.RFC (cast to number?)
  2. Provide QA examples of both devpool issues and RFC issues that clearly shows that the code is working as expected
  3. Resolve code conflicts

@xno-miner
Copy link
Contributor Author

1. Fix `process.env.RFC` (cast to number?)

I created a variable IS_RFC in github.ts which checks if the env variable is equal to "1". To make it consistent i also created variables DEVPOOL_OWNER_NAME and DEVPOOL_REPO_NAME which just load from the env (it's also how it was before i moved them to env)

2. Provide QA examples of both devpool issues and RFC issues that clearly shows that the code is working as expected

I have created a test suite for RFC issues, based on the test suite for devpool, and it appears to run successfully

------------|---------|----------|---------|---------|-------------------------------------------------------------------------------
File        | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                                                             
------------|---------|----------|---------|---------|-------------------------------------------------------------------------------
All files   |   62.73 |    60.58 |   57.77 |   64.19 |                                                                               
 github.ts  |   64.25 |    61.29 |   60.46 |   66.06 | 62-69,114,183-187,233,250-255,291-345,349-391,419-420,425-437,498,594,629,642 
 twitter.ts |   45.45 |    53.33 |       0 |   45.45 | 12,29-47                                                                      
------------|---------|----------|---------|---------|-------------------------------------------------------------------------------
Test Suites: 1 passed, 1 total
Tests:       43 passed, 43 total
Snapshots:   0 total
Time:        5.518 s
Ran all test suites matching /rfc-issue-handler/i.
Done in 6.00s.

I disabled it from being ran automatically for now, beacause it requires a different .env, but once the repository will be duplicated there will be 2 .envs and this won't be an issue. The instructions to run the test suite with the regular .env are in the test suite file (tests/rfc-issue-handler.test.ts.skip).

3. Resolve code conflicts

Done. Just had to move the statistics logic in index.ts.

helpers/github.ts Outdated Show resolved Hide resolved
@rndquu
Copy link
Member

rndquu commented Jul 30, 2024

@xno-miner From rfc-issue-handler.test.ts.skip:

// To Run:
// - Remove .skip from the file name
// - Comment the "Usually" segment in helpers/github.ts and uncomment the "rfc-issue-handler.test.ts" segment
// - Run `yarn test rfc-issue-handler`
  1. Why rfc-issue-handler tests are skipped?
  2. Why do we need to comment some code in helpers/github.ts and uncomment some other part in order to run the tests? Can't we mock the environment in the tests?
  3. The errors in this CI run are typescript compile errors and are not related to empty env variables.

@xno-miner
Copy link
Contributor Author

Ok. I managed to fix this. I just needed to add exclamation marks after env variables to unwrap them. Now there are no compile errors when testing.

I also enabled rfc-issue-handler tests, with modified env variables (which now worked because there were no compile errors).

Comment on lines +10 to +12
export const DEVPOOL_OWNER_NAME = process.env.DEVPOOL_OWNER_NAME!;
export const DEVPOOL_REPO_NAME = process.env.DEVPOOL_REPO_NAME!;
export const IS_RFC = JSON.parse(process.env.RFC!);
Copy link
Member

Choose a reason for hiding this comment

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

Non-null assertion operator is considered a bad practice because if the value is null then the app fails while the compiler is trying to make sure that even if the value is null the app works as expected (or at least throws meaningful errors). The value! syntax is not used anywhere in the codebase. Pls refactor accordingly.

Copy link
Member

@0x4007 0x4007 Aug 6, 2024

Choose a reason for hiding this comment

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

I imagine that the linter settings are not in sync with our template here, given that this passed CI.

Perhaps this is technically our responsibility to handle these sort of problems.

Copy link
Member

Choose a reason for hiding this comment

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

I imagine that the linter settings are not in sync with our template here, given that this passed CI.

Perhaps this is technically our responsibility to handle these sort of problems.

Seems you've already created a related issue ubiquity/ts-template#54

@xno-miner
Copy link
Contributor Author

Closing in favor of #1250

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.

3 participants