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(attributes): use frequency alongside isCommon #576

Merged
merged 9 commits into from
Mar 27, 2023
Merged

feat(attributes): use frequency alongside isCommon #576

merged 9 commits into from
Mar 27, 2023

Conversation

Effiti
Copy link
Contributor

@Effiti Effiti commented Mar 14, 2023

this can be the first step in an effort towards deprecating isCommon

Copy link
Collaborator

@ijemmao ijemmao left a comment

Choose a reason for hiding this comment

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

really good stuff! super close

src/shared/constants/WordAttributes.js Outdated Show resolved Hide resolved
migrations/20230314160556-convert-frequency-to-number.js Outdated Show resolved Hide resolved
migrations/20230314160556-convert-frequency-to-number.js Outdated Show resolved Hide resolved
@ijemmao
Copy link
Collaborator

ijemmao commented Mar 16, 2023

@Effiti LGTM just to confirm, did you run this migration locally to see if it works as expected?

@Effiti
Copy link
Contributor Author

Effiti commented Mar 16, 2023

@Effiti LGTM just to confirm, did you run this migration locally to see if it works as expected?

@ijemmao having some problems with starting using docker,
the mongodb docker image doesn't work, but it is seemingly unrelated to the PR.

@ijemmao
Copy link
Collaborator

ijemmao commented Mar 17, 2023

@Effiti you don't need to run docker! you can just follow the testing portion of the README

@Effiti
Copy link
Contributor Author

Effiti commented Mar 18, 2023

still having some problems with connecting to the database:
running yarn migrate-up
and mongod --port 27017 --dbpath ./db simultaneously in the repo gives the following:

(somehow it works now)
after the following commit, it worked for me

@ijemmao
Copy link
Collaborator

ijemmao commented Mar 20, 2023

@Effiti it seems like our pipeline is blocked by a weird yarn error! I've commented on the related thread to see if there's any solution to the bug: yarnpkg/yarn#8580 (comment)

If we don't hear back soon (by Wednesday at the very latest), I'll consider switching from yarn to npm so we can unblock deploys

@ijemmao
Copy link
Collaborator

ijemmao commented Mar 23, 2023

@Effiti it looks like our README didn't walk through an extra necessary step to contribute (apologies!) here's the guide

@Effiti Effiti requested a review from ijemmao March 24, 2023 15:49
@Effiti
Copy link
Contributor Author

Effiti commented Mar 24, 2023

@ijemmao do I have to close and reopen the PR?

I followed the instructions but the CI is still not working. Here is a screenshot of the settings page:
screenshot

@ijemmao
Copy link
Collaborator

ijemmao commented Mar 25, 2023

@Effiti hmm yeah this is odd :/ i did find this thread that could be related:

https://github.com/orgs/community/discussions/26283

(thanks for your patience, i'm gonna fork my own repository to make a change to see if i can fix the bug locally)

@Effiti
Copy link
Contributor Author

Effiti commented Mar 25, 2023

I set up ssh authentication for git on my machine (i think that's what the discussion was referring to), even though I already had GPG-Signing set up for git before.

I'm just gonna keep fixing minor issues in the README in order to trigger the CI until the issue is resolved 😄.

@ijemmao ijemmao merged commit 0b198f4 into nkowaokwu:master Mar 27, 2023
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