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

Implemented theme days for tAngled 22/23 #21

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

praveenkrishna0512
Copy link

No description provided.


// TODO: Change this according to the different theme days
// FORMAT: [dayNum, monthNum]
ctx.themeDays = [[27, 8], [28, 8], [29, 8], [30, 8], [31, 8], [1, 9]]
Copy link
Member

Choose a reason for hiding this comment

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

Could we put this as a json in the env file or in a config.js file? We shouldn't include magic constants in logic files

if (!ctx.isRegistered) {
return ctx.reply(messages.NotRegistered)
}
var person = ctx.person
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason for this to be var instead of const? It might be even better to inline this

return ctx.reply(messages.NotRegistered)
}
var person = ctx.person
if (person.confirm) {
Copy link
Member

Choose a reason for hiding this comment

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

If i'm understanding your intention correctly (boolean var for today's theme), I would instead name this ctx.person.confirmedToday

  • change variable name to be more informative
  • inlined ctx

person.confirm = true
model.saveToStorage()

const other = ctx.isAngel ? ctx.angel : ctx.mortal
Copy link
Member

Choose a reason for hiding this comment

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

I think this is already done by middleware? (So it should be available as a field of ctx) If not, we could put this into middleware

@@ -40,6 +42,10 @@ async function start(model) {
bot.on('message', Commands.MessageHandler)
//TODO: handle sending files
bot.launch().then(() => console.log(bot._name + " started")).catch(console.error)

// Enable graceful stop
process.once('SIGINT', () => bot.stop('SIGINT'));
Copy link
Member

Choose a reason for hiding this comment

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

Could separate this out into another PR, cos this would be good in main branch but we may not want to include the tangles specific stuff

}
// Cell contains fun fact about the person
if (colNumber > 2 && colNumber < 8) {
angel.facts.push(cell.value)
Copy link
Member

Choose a reason for hiding this comment

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

this implicitly assumes the eachCell iteration order. It would be better defensive programming to construct the person with a fixed size array, then index that array based on colNumber

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