Skip to content

Create PR Template, review guidelines clarification, potential status update suggestion. - #762

Open
JamesVanBoxtel wants to merge 1 commit into
panel-attack:betafrom
JamesVanBoxtel:review/PRTemplate
Open

JamesVanBoxtel wants to merge 1 commit into
panel-attack:betafrom
JamesVanBoxtel:review/PRTemplate

Conversation

@JamesVanBoxtel

Copy link
Copy Markdown
Contributor

Problem

We are trying to make PRs easier to review and document our process more. This is a step in that direction.

Solution

Add a PR template
Add pull request review guidelines
Add status updates suggestion

Why this approach

This is just to get the discussion started, we can iterate if we think the template needs more, less, or rewording.

Testing

None, just process documentation

Add a PR template
Add pull request review guidelines
Add status updates suggestion

@Endaris Endaris left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If we do small chunks of PRs, we probably also want to mention in PRs which feature they are contributing to. For example with #758 I could not really tell what it was needed for.

- Any required prerequisite is its own PR, opened first and targeted by this PR
- Tests pass and I've tested in the areas changed.
- New tests added where it makes sense
- No new Lua Language Server diagnostics

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

"No new Lua Language Server diagnostics" is currently a bit in conflict with the contribution guidelines.
Originally annotations were added for intellisense and IDE navigation and specifically not for correctness.
I believe at some point you tried to cut down on errors through a combination of fixing some annotations and simply excluding others.
Personally I think it is rather cumbersome to write Lua in a manner that the language server does not turn up false positives or at least not without giving up some of the aspects that make Lua easy to use.

Notably I think in some scenarios this may discourage annotating as no annotations means the LLS interprets everything as anyand flags no warnings/problems. When you actually annotate something I believe you're more likely to run into warnings caused by the shortcomings of the LLS itself, particularly in relation to union types / generics. There's a bit of a conflict of interest here but personally I'd rather have some annotations and some warnings rather than no/hacky annotations to make the diagnostics happy.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm open to suggestions but I'd prefer just turning off the ones we don't want if we think they are a burden.

The only one I regularly run into I'm not sure adds value is the "duplicate" method one. So I would suggest we turn that one off and leave this recommendation. I believe we also have a PR check for this?

But I'm flexible here. If your suggestion is just to remove this line I'm fine with that, just let me know.

@JamesVanBoxtel

Copy link
Copy Markdown
Contributor Author

#758 is only loosely related to #113 but definitely when its obvious what your building towards we should link it.

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