PR review discussion

[MokhaLeee]

There has been a generial contributing note on this repo. Unfortunately, there are still some brilliant submissions are rejected. I think there are 2 reasons for this:

1. The current contributing rules are not particularly clear.
2. For non-computer science students, code standards and code reviews are not familiar things, thus lacking in consideration.

The current standard is origined from decomp discussion, which is consistent on fe6 (Code in fireemblem8u decomp also failed to fulfill the standard because we contriubutors didn't discuss these issues initially).

Unified coding style and strict code review are crucial to maintaining code reliability, thus the standards on PR review will not be lowered to take care of non-CS majored contributors. But that doesn't mean I have to stick to my own standards, so I decide to make a post to discuss on PR review. I will post some existing discussions under this thread, and welcome other contributors to put forward their opinions for discussion.

[MokhaLeee]

https://github.com/MokhaLeee/fe8u-cskillsys-kernel/pull/201

This is a rejected PR posted by genius @Veslyquix

Although the new feature it introduced is extremely innovative, this PR was rejected due to the following reasons:

1. The code style does not meet the requirements, and the code in different locations are also not uniform in style. And the author refused to make changes to this.
2. The code itself does not respect the existing outputs of decomp. For example, struct TalkEventCond should be replaced by vanilla definition struct EvCheck03 in decomp, and the implementation on function _GetTalkee is also inconsistent with the vanilla routine CheckForCharacterEvents.

[MokhaLeee]

https://github.com/MokhaLeee/fe8u-cskillsys-kernel/pull/239 https://github.com/MokhaLeee/fe8u-cskillsys-kernel/pull/240

These are two rejected PR on new skills posted by @JesterWizard. Both rejection of two PRs have a common reason: the skills effects are only meaningful under specific plots. As a result, these skills are meaningless to most people.

In my opinion, the kernel should limit its functionality to meet the basic needs of the vast majority of users. This is also necessary to maintain code reliability. And custom features should be adapted by users themselves rather than introduced into community project.

There are also some problems on code quality, which have been raised inside PRs.

[MokhaLeee]

https://github.com/MokhaLeee/fe8u-cskillsys-kernel/pull/60

There are some code style issues in this PR by @AliScrooge and get merged after repair. This is why I recommend using vscode rather than notepad++, which cannot automatically identify existing code styles and assist developers to comply with them.

[MokhaLeee]

https://github.com/MokhaLeee/fe8u-cskillsys-kernel/commit/c320a08edefcbe7cde69ac503386c3a50df1ab8d

This is a commit posted by myself to remove statscreenfx but now reverted.

For the short term I plan to keep them. The reason is obvious: compared to quality and performance, it is obviously that users may pay more attention to the richness of functions and the UI. But at some point in the future, I will definitely remove features that have nothing to do with skills. I think the skillsys kernel should properly handle on all skill-related functions, and not participate in any additional work. (which mainly lies in External dir).

[Veslyquix]

I think it's easier to make some direct edits to fix minor issues on a pull request than to comment on it describing what's wrong with it.

[AliScrooge]

1. On the subject of code styling: while there should be rules that impose general style, at least with block handling and indentation level, obsessing over whenever one should use if () over if() or res+=1 over res += 1 is, in my opinion, useless. The two parse exactly the same in term of code readability. Much more important, again in my opinion, is the expected level of code documentation and how much should one focus on code readability and clarity versus optimization. If strict code style remain a focus, then some sort of automatization inthe form of a Linter, a VSC extension or automatic code review in Github should be implemented

2. I do not think it is really the place of the project to choose which skill is useful enough to be included. This should be left to the user, AKA the FE romhacking community. Maybe we should create a thread on the forum for this purpose (and make a general re-balance sweep at the same time)

3. While this project should remain dedicated to the skill system, it should strive to be easily compatible with various other addition to the game. In the end, the goal of this system is to be used in hack project. It would a shame if most members were to stick to the EA version because it has more functionality

This is just my opinion on the subject

[MokhaLeee]

direct edits to fix minor issues on a pull request than to comment on it describing what's wrong with it

I think that controdutors themself should be responsible to the code they posted. Sometimes I direcly fix the PR after merging them is because the standard not clear enough rather than it should be so. That's just the reason why I post this thread to make clear on standard.

[MokhaLeee]

obsessing over whenever one should use if () over if() or res+=1 over res += 1 is, in my opinion, useless

Yes, it doesn’t matter what the standard is, what’s important is that there needs to be a unified standard. So if we reach consensus to adopt the latter, then the PR on style of the former should also be modified. However, if even the basic coding style cannot be consistent, then the reliability of the project itself should be doubted.

some sort of automatization inthe form of a Linter, a VSC extension or automatic code review in Github should be implemented

That right. I think introduing CI build will be one of task in the future