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.

Sign up for GitHub

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

Jump to bottom

Check language files and database schema with app code checker #4767

Merged
merged 8 commits into from
May 16, 2017
Merged

Check language files and database schema with app code checker #4767

merged 8 commits into from
May 16, 2017

Conversation

nickvergessen
Copy link
Member

@nickvergessen nickvergessen commented May 9, 2017 •
edited
Loading

Brings #4485 to apps

@nickvergessen nickvergessen added this to the Nextcloud 12.0 milestone May 9, 2017
@nickvergessen nickvergessen changed the title App code checker Check language files and database schema with app code checker May 9, 2017
@codecov
Copy link

codecov bot commented May 9, 2017 •
edited
Loading

Codecov Report

Merging #4767 into master will decrease coverage by 0.04%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master    #4767      +/-   ##
============================================
- Coverage     54.17%   54.12%   -0.05%     
- Complexity    22174    22201      +27     
============================================
  Files          1365     1367       +2     
  Lines         84886    84959      +73     
  Branches       1322     1322              
============================================
- Hits          45985    45983       -2     
- Misses        38901    38976      +75
Impacted Files Coverage Δ Complexity Δ
.../private/App/CodeChecker/DatabaseSchemaChecker.php 0% <0%> (ø) 18 <18> (?)
...b/private/App/CodeChecker/LanguageParseChecker.php 0% <0%> (ø) 6 <6> (?)
core/Command/App/CheckCode.php 0% <0%> (ø) 28 <0> (+3) ⬆️
lib/private/Security/CertificateManager.php 90.81% <0%> (-1.03%) 39% <0%> (ø)
core/js/js.js 61.21% <0%> (-0.57%) 0% <0%> (ø)
lib/private/Server.php 93.4% <0%> (+0.14%) 120% <0%> (ø) ⬇️
lib/private/Files/Cache/Propagator.php 96.2% <0%> (+1.26%) 16% <0%> (ø) ⬇️
apps/comments/lib/EventHandler.php 87.5% <0%> (+8.33%) 7% <0%> (ø) ⬇️

@nickvergessen nickvergessen added 2. developing Work in progress and removed 3. to review Waiting for reviews labels May 10, 2017
@nickvergessen nickvergessen mentioned this pull request May 10, 2017
Merged
@nickvergessen nickvergessen added 3. to review Waiting for reviews and removed 2. developing Work in progress labels May 10, 2017
nickvergessen
nickvergessen commented May 10, 2017
View reviewed changes
autotest-checkers.sh Outdated
else
./occ app:check-code "$app"
fi
RESULT=$?
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure this exits correctly, I guess we need to merge the result codes?

Copy link
Member

Choose a reason for hiding this comment

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

It will just take the last code

Copy link
Member

Choose a reason for hiding this comment

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

RESULT=$(($RESULT+$?))

Just do that below each one... and return that at the end ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed, thanks

rullzer
rullzer approved these changes May 10, 2017
View reviewed changes
.drone.yml Outdated
- ./occ app:check-code systemtags
- ./occ app:check-code theming
- ./occ app:check-code workflowengine
- ./autotest-checkers.sh
Copy link
Member

Choose a reason for hiding this comment

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

Why a single job?

Copy link
Member Author

Choose a reason for hiding this comment

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

it was before, but now devs can run this job in a simple fashion aswell

autotest-checkers.sh Outdated
else
./occ app:check-code "$app"
fi
RESULT=$?
Copy link
Member

Choose a reason for hiding this comment

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

RESULT=$(($RESULT+$?))

Just do that below each one... and return that at the end ;)

@nickvergessen nickvergessen force-pushed the app-code-checker branch 2 times, most recently from dd86638 to d275a21 Compare May 10, 2017 13:00
@nickvergessen
Check the syntax of the language file
93fc7fc 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
Check the name length of database items
31bb65f 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
Fix apps without translations
8e757b3 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
We don't use the prefix on index names
1951c88 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
Run the app checker on all apps
3571355 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
Fix return code handling
7ec5847 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
Skip the code checking for now
f920dfe 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
Copy link
Member Author

Rebased and ready to review

MorrisJobke
MorrisJobke reviewed May 16, 2017
View reviewed changes
autotest-checkers.sh
else
./occ app:check-code "$app"
fi
RESULT=$(($RESULT+$?))
Copy link
Member

Choose a reason for hiding this comment

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

This is not really helpful, because the log doesn't show what is the cause for the failure 🙈

@MorrisJobke
Copy link
Member

The file runs fine locally. :/

MorrisJobke
MorrisJobke reviewed May 16, 2017
View reviewed changes
core/Command/App/CheckCode.php
@@ -70,6 +72,12 @@ protected function configure() {
[ 'private', 'deprecation', 'strong-comparison' ]
)
->addOption(
'--skip-checkers',
Copy link
Member

Choose a reason for hiding this comment

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

Nice that you added this, but it isn't evaluated in the code below 🙈

Copy link
Member

Choose a reason for hiding this comment

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

I fixed this.

@MorrisJobke
Implement skip checkers and log the currently tested app
6dea5e6 
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
@MorrisJobke MorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels May 16, 2017
MorrisJobke
MorrisJobke reviewed May 16, 2017
View reviewed changes
autotest-checkers.sh
RESULT=$(($RESULT+$?))


for app in $(find "apps/" -mindepth 1 -maxdepth 1 -type d -exec basename {} \;); do
Copy link
Member

@MorrisJobke MorrisJobke May 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

And I made this compatible with the UNIX find (the previous was only for GNU find which didn't worked on UNIX)

@MorrisJobke MorrisJobke merged commit 3a70ebf into master May 16, 2017
@MorrisJobke MorrisJobke deleted the app-code-checker branch May 16, 2017 21:20
@LukasReschke
Copy link
Member

This broke the updater… The release script has to remove that file from the built… I will adjust but all users on existing betas will have to manually delete this file…

@LukasReschke LukasReschke mentioned this pull request May 18, 2017
Closed
@nickvergessen
Copy link
Member Author

Sorry :/ awkward

@yesiamdollar yesiamdollar mentioned this pull request Nov 9, 2023
Open
This was referenced Apr 8, 2024
Open
Open
@yesiamdollar yesiamdollar mentioned this pull request May 13, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MorrisJobke MorrisJobke MorrisJobke approved these changes

@rullzer rullzer rullzer approved these changes

Assignees
No one assigned
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement technical debt
Projects
None yet
Milestone
Nextcloud 12.0
Development

Successfully merging this pull request may close these issues.
4 participants
@nickvergessen @MorrisJobke @LukasReschke @rullzer