-
Notifications
You must be signed in to change notification settings - Fork 0
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
staging -> main #137
staging -> main #137
Conversation
…ll-las-to-database PC-1363: add script to add all las to database
Dev -> Staging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The one thing I've found isn't worth postponing a production release if we've already arranged one, but I would like a PR to fix it to be raised. That way it won't be forgotten about.
if (confirmation) | ||
adminAction.AddMissingAuthoritiesToDatabase(); | ||
else | ||
outputProvider.Output("No changes made."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if
s without braces always make me scared. Any chance we could add some in here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
linter requirement alas
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤨
I would turn off that rule in the linter rather than follow it personally. I realise I'm not the actual TL for the project but in my opinion this rule is only leaving yourself open to potential bugs in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps; the linter rules are not something we've reviewed much before, so can raise a ticket for this. it'd need some work to ensure we're using the same rules for everyone, right now we all agree to use rider's default
can raise a ticket for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have done so at https://beisdigital.atlassian.net/browse/PC-1508
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For what it's worth I agree with this
No description provided.