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.

Sign up for GitHub

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

Jump to bottom

Add join/leave data #205

Closed
wants to merge 3 commits into from
Closed

Add join/leave data #205

wants to merge 3 commits into from

Conversation

jejebecarte
Copy link
Member

No description provided.

@jejebecarte
Copy link
Member Author

jejebecarte commented Apr 2, 2024 •
edited
Loading

Okay, now that we're here, please note the following for the future:

  • Refrain from committing directly to master, for the following reasons:

    • If you are committing incomplete code, it does not go to prod until finished
    • If for whatever reason tests fail (as above), that can be remedied before pushing to prod. In future I will separate lint/styling checks from build checks to make our lives easier.
    • It gives me a chance to review things before they go in

  • Please also create pull requests before merging branches.

  • You can create a new branch and checkout your changes to it using git checkout -b your-branch-name

jejebecarte
jejebecarte commented Apr 2, 2024
View reviewed changes
Copy link
Member Author

@jejebecarte jejebecarte left a comment

Choose a reason for hiding this comment

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

Seems overall okay - although didn't really look through the meat of commits. Don't see why this should be on main branch however.

package.json
@@ -22,19 +22,19 @@
"dependencies": {
Copy link
Member Author

Choose a reason for hiding this comment

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

Please only update dependencies if strictly necessary - it tends to break things and be more trouble than it's worth.

Dependabot alerts are disabled in GH but still seem to be happening. Will look into that.

joinDate.json
@@ -0,0 +1,8 @@
[
Copy link
Member Author

Choose a reason for hiding this comment

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

Refrain from committing idiosyncratic files to git. Add files to the .gitignore as appropriate. Also please move this to a more appropriate location, not the root folder.

src/classes/Bot.ts
logErrors: true,
hideErrors: true,
checkTimeoutInterval: 30000,
defaultChatPatterns: false,
Copy link
Member Author

Choose a reason for hiding this comment

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

Why have you removed this config option? This option is present to prevent listeners from being created unnecessarily - we do not use the defaults. See here.

src/classes/Bot.ts
@@ -34,11 +34,10 @@ class Bot {
password: process.env.MINECRAFT_PASSWORD,
host: "mc.hypixel.net",
auth: "microsoft",
version: "1.16.4",
version: "1.8.9",
Copy link
Member Author

Choose a reason for hiding this comment

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

Why have you changed the version? AFAIK, this only reduces our chat character limit from 256 -> 100 (changed in 1.12)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@jejebecarte