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

Add pre-commit hook #867

Merged
merged 9 commits into from
Jul 27, 2023
Merged

Add pre-commit hook #867

merged 9 commits into from
Jul 27, 2023

Conversation

LuisDuarte1
Copy link
Member

Closes #854

Review checklist

  • Terms and conditions reflect the current change
  • Contains enough appropriate tests
  • If aimed at production, writes a new summary in whatsnew/whatsnew-pt-PT
  • Properly adds an entry in changelog.md with the change
  • If PR includes UI updates/additions, its description has screenshots
  • Behavior is as expected
  • Clean, well-structured code

@bdmendes
Copy link
Member

This will format auto-generated dart files too. Is that the desired behaviour?
Also, what do you think of tackling #225 alongside this PR?

uni/README.md Outdated Show resolved Hide resolved
@LuisDuarte1
Copy link
Member Author

LuisDuarte1 commented Jul 22, 2023

This will format auto-generated dart files too. Is that the desired behaviour? Also, what do you think of tackling #225 alongside this PR?

Nice catch, i'll add it to the grep filter command. Creating a contributing.md seems a bit out of scope for this pr

bdmendes
bdmendes previously approved these changes Jul 22, 2023
Copy link
Member

@bdmendes bdmendes left a comment

Choose a reason for hiding this comment

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

Lgtm!

@Sirze01
Copy link
Contributor

Sirze01 commented Jul 22, 2023

This will format auto-generated dart files too. Is that the desired behaviour? Also, what do you think of tackling #225 alongside this PR?

I think this should be the desired behaviour; if those are indeed dart files, they should be following the standard formatting convention. This would also simplify the hook as you would only need to run the formatter in the /lib directory. (I have no idea if the savings of only analyzing changed files are worth it, but it is already done so...)
Another aspect I think you may have overlooked is the configuration of the linting action; isn't the analyzer looking for all dart files?

@LuisDuarte1
Copy link
Member Author

This will format auto-generated dart files too. Is that the desired behaviour? Also, what do you think of tackling #225 alongside this PR?

I think this should be the desired behaviour; if those are indeed dart files, they should be following the standard formatting convention. This would also simplify the hook as you would only need to run the formatter in the /lib directory. (I have no idea if the savings of only analyzing changed files are worth it, but it is already done so...) Another aspect I think you may have overlooked is the configuration of the linting action; isn't the analyzer looking for all dart files?

Because they are auto generated they shouldn't be changed at all, even though they can conflict with our formatting standards, because they can and probably will regenerate themselves (and be an hassle every time you save them or you commit them).

@Sirze01
Copy link
Contributor

Sirze01 commented Jul 23, 2023

This will format auto-generated dart files too. Is that the desired behaviour? Also, what do you think of tackling #225 alongside this PR?

I think this should be the desired behaviour; if those are indeed dart files, they should be following the standard formatting convention. This would also simplify the hook as you would only need to run the formatter in the /lib directory. (I have no idea if the savings of only analyzing changed files are worth it, but it is already done so...) Another aspect I think you may have overlooked is the configuration of the linting action; isn't the analyzer looking for all dart files?

Because they are auto generated they shouldn't be changed at all, even though they can conflict with our formatting standards, because they can and probably will regenerate themselves (and be an hassle every time you save them or you commit them).

In that case, take a look at the linting config to make sure those files are ignored.

@LuisDuarte1
Copy link
Member Author

LuisDuarte1 commented Jul 23, 2023

In that case, take a look at the linting config to make sure those files are ignored.

It's not on develop, but #863 already ignores them

@Sirze01
Copy link
Contributor

Sirze01 commented Jul 24, 2023

It's not on develop, but #863 already ignores them

That's great and also needed, but I meant if the formatting check action is ignoring the .g.dart files.

@LuisDuarte1
Copy link
Member Author

That's great and also needed, but I meant if the formatting check action is ignoring the .g.dart files.

Nice catch! Changed the action to only include user created files

Copy link
Member

@thePeras thePeras left a comment

Choose a reason for hiding this comment

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

Running the .sh script doesn't work if there is no hooks folder inside .git folder.
I created the folder, and works fine. Can the .sh create the folder automatically?

Also, the .sh is in the git changed files because it changed the mode from 100644 to 100755. Any idea to ignore that?

@LuisDuarte1
Copy link
Member Author

LuisDuarte1 commented Jul 27, 2023

Running the .sh script doesn't work if there is no hooks folder inside .git folder. I created the folder, and works fine. Can the .sh create the folder automatically?

Also, the .sh is in the git changed files because it changed the mode from 100644 to 100755. Any idea to ignore that?

I can't seem to replicate this, on my pc, according to the git documentation (link) it automatically populates .git/hooks directory with sample hooks, so the directory is automatically created. I can't also replicate the .sh changed mode on linux. I'm using git version 2.40.1

@LuisDuarte1
Copy link
Member Author

I can track down the file mode change since I use an NTFS partition on a linux distro (in order to work on both enviroments) git sets git config core.filemode to false (as it can be explained here) because NTFS on linux has always the executable permission on. I'll update it's index manually

@LuisDuarte1
Copy link
Member Author

LuisDuarte1 commented Jul 27, 2023

It seems, upon further research that there are some git installations that don't populate the .git/hooks folder automatically because they have no default templates or there are some older versions/other git clients that don't do this, so I've added a failsafe, just in case this happens again

thePeras
thePeras previously approved these changes Jul 27, 2023
Copy link
Contributor

@Sirze01 Sirze01 left a comment

Choose a reason for hiding this comment

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

Nicely done!

@LuisDuarte1 LuisDuarte1 merged commit 9ff3d34 into develop Jul 27, 2023
4 checks passed
@LuisDuarte1 LuisDuarte1 deleted the feature/pre-commit-format branch July 27, 2023 22:58
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.

Create a format hook
4 participants