Skip to content

Commit

Permalink
Report removal of final newline
Browse files Browse the repository at this point in the history
If a file does not end in a newline, then every line added to the end of the file results in a diff to the previous final
line.

An example using `\n` to symbolize end of line characters:

https://github.com/arduino-libraries/Servo\n
https://github.com/arduino-libraries/Stepper

When another line is added at the end:

https://github.com/arduino-libraries/Servo\n
https://github.com/arduino-libraries/Stepper\n
https://github.com/arduino-libraries/FooBar

you can see there is necessarily a diff on line 2 even though the editor did not touch that line.

For this reason, it is best practices to always retain a newline at the end of files.

In addition to it being common practice, the GitHub web editor automatically adds this newline. Between this and my
expectation that people would either add URLs to the top of the list or in alphabetical order, leaving the distant end of
the file alone, I was hoping that this would not be a common problem with the library submission system, but this is
simply not a realistic expectation.

What I hadn’t considered is that the person to suffer for this formatting is the next to add a URL to the end of the
file. Because of this “ghost diff”, the submission system sees this PR as a modification to an existing item on the list
rather than an addition.

I think this will be best handled by detecting and dealing with the problem at its introduction, rather than attempting
to make the system resilient to a missing final newline.
  • Loading branch information
per1234 committed Jul 24, 2021
1 parent 8252c14 commit 0e0f23e
Show file tree
Hide file tree
Showing 5 changed files with 77 additions and 23 deletions.
17 changes: 12 additions & 5 deletions main.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ type request struct {
Submissions []submissionType `json:"submissions"` // Data for submitted libraries.
IndexEntry string `json:"indexEntry"` // Entry that will be made to the Library Manager index source file when the submission is accepted.
IndexerLogsURLs string `json:"indexerLogsURLs"` // List of URLs where the logs from the Library Manager indexer for each submission are available for view.
Error string `json:"error"` // Error message.
}

// submissionType is the type of the data for each individual library submitted in the request.
Expand Down Expand Up @@ -127,7 +128,7 @@ func main() {
}
var req request
var submissionURLs []string
req.Type, req.ArduinoLintLibraryManagerSetting, submissionURLs = parseDiff(rawDiff, *listNameArgument)
req.Type, req.Error, req.ArduinoLintLibraryManagerSetting, submissionURLs = parseDiff(rawDiff, *listNameArgument)

// Process the submissions.
var indexEntries []string
Expand Down Expand Up @@ -177,18 +178,24 @@ func errorExit(message string) {
os.Exit(1)
}

// parseDiff parses the request diff and returns the request type, `arduino-lint --library-manager` setting, and list of submission URLs.
func parseDiff(rawDiff []byte, listName string) (string, string, []string) {
// parseDiff parses the request diff and returns the request type, request error, `arduino-lint --library-manager` setting, and list of submission URLs.
func parseDiff(rawDiff []byte, listName string) (string, string, string, []string) {
var submissionURLs []string

// Check if the PR has removed the final newline from a file, which would cause a spurious diff for the next PR if merged.
// Unfortunately, the diff package does not have this capability (only to detect missing newline in the original file).
if bytes.Contains(rawDiff, []byte("\\ No newline at end of file")) {
return "invalid", "Pull request removes newline from the end of a file.%0APlease add a blank line to the end of the file.", "", nil
}

diffs, err := diff.ParseMultiFileDiff(rawDiff)
if err != nil {
panic(err)
}

if (len(diffs) != 1) || (diffs[0].OrigName[2:] != listName) || (diffs[0].OrigName[2:] != diffs[0].NewName[2:]) { // Git diffs have a a/ or b/ prefix on file names.
// This is not a Library Manager submission.
return "other", "", nil
return "other", "", "", nil
}

var addedCount int
Expand Down Expand Up @@ -227,7 +234,7 @@ func parseDiff(rawDiff []byte, listName string) (string, string, []string) {
arduinoLintLibraryManagerSetting = "update"
}

return requestType, arduinoLintLibraryManagerSetting, submissionURLs
return requestType, "", arduinoLintLibraryManagerSetting, submissionURLs
}

// populateSubmission does the checks on the submission that aren't provided by Arduino Lint and gathers the necessary data on it.
Expand Down
32 changes: 20 additions & 12 deletions main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,9 @@ index cff484d..e14c179 100644
+https://github.com/foo/bar
`)

requestType, arduinoLintLibraryManagerSetting, submissionURLs := parseDiff(diff, "repositories.txt")
requestType, requestError, arduinoLintLibraryManagerSetting, submissionURLs := parseDiff(diff, "repositories.txt")
assert.Equal(t, "other", requestType, testName)
assert.Equal(t, "", requestError, testName)
assert.Equal(t, "", arduinoLintLibraryManagerSetting, testName)
assert.Nil(t, submissionURLs, testName)

Expand All @@ -62,8 +63,9 @@ index d4edde0..807b76d 100644
+hello
`)

requestType, arduinoLintLibraryManagerSetting, submissionURLs = parseDiff(diff, "repositories.txt")
requestType, requestError, arduinoLintLibraryManagerSetting, submissionURLs = parseDiff(diff, "repositories.txt")
assert.Equal(t, "other", requestType, testName)
assert.Equal(t, "", requestError, testName)
assert.Equal(t, "", arduinoLintLibraryManagerSetting, testName)
assert.Nil(t, submissionURLs, testName)

Expand All @@ -80,8 +82,9 @@ index cff484d..e14c179 100644
+https://github.com/foo/bar
`)

requestType, arduinoLintLibraryManagerSetting, submissionURLs = parseDiff(diff, "repositories.txt")
requestType, requestError, arduinoLintLibraryManagerSetting, submissionURLs = parseDiff(diff, "repositories.txt")
assert.Equal(t, "other", requestType, testName)
assert.Equal(t, "", requestError, testName)
assert.Equal(t, "", arduinoLintLibraryManagerSetting, testName)
assert.Nil(t, submissionURLs, testName)

Expand All @@ -96,8 +99,9 @@ index cff484d..9f67763 100644
+https://github.com/foo/baz
`)

requestType, arduinoLintLibraryManagerSetting, submissionURLs = parseDiff(diff, "repositories.txt")
requestType, requestError, arduinoLintLibraryManagerSetting, submissionURLs = parseDiff(diff, "repositories.txt")
assert.Equal(t, "submission", requestType, testName)
assert.Equal(t, "", requestError, testName)
assert.Equal(t, "submit", arduinoLintLibraryManagerSetting, testName)
assert.ElementsMatch(t, submissionURLs, []string{"https://github.com/foo/bar", "https://github.com/foo/baz"}, testName)

Expand All @@ -112,10 +116,11 @@ index cff484d..1b0b80b 100644
\ No newline at end of file
`)

requestType, arduinoLintLibraryManagerSetting, submissionURLs = parseDiff(diff, "repositories.txt")
assert.Equal(t, "submission", requestType, testName)
assert.Equal(t, "submit", arduinoLintLibraryManagerSetting, testName)
assert.ElementsMatch(t, submissionURLs, []string{"https://github.com/foo/bar"}, testName)
requestType, requestError, arduinoLintLibraryManagerSetting, submissionURLs = parseDiff(diff, "repositories.txt")
assert.Equal(t, "invalid", requestType, testName)
assert.Equal(t, "Pull request removes newline from the end of a file.%0APlease add a blank line to the end of the file.", requestError, testName)
assert.Equal(t, "", arduinoLintLibraryManagerSetting, testName)
assert.Nil(t, submissionURLs, testName)

testName = "Submission w/ blank line"
diff = []byte(`
Expand All @@ -125,11 +130,12 @@ index cff484d..1b0b80b 100644
+++ b/repositories.txt
@@ -3391,0 +3392 @@ https://github.com/lbernstone/plotutils
+https://github.com/foo/bar
\ No newline at end of file
+
`)

requestType, arduinoLintLibraryManagerSetting, submissionURLs = parseDiff(diff, "repositories.txt")
requestType, requestError, arduinoLintLibraryManagerSetting, submissionURLs = parseDiff(diff, "repositories.txt")
assert.Equal(t, "submission", requestType, testName)
assert.Equal(t, "", requestError, testName)
assert.Equal(t, "submit", arduinoLintLibraryManagerSetting, testName)
assert.ElementsMatch(t, submissionURLs, []string{"https://github.com/foo/bar"}, testName)

Expand All @@ -143,8 +149,9 @@ index cff484d..38e11d8 100644
-https://github.com/arduino-libraries/Ethernet
`)

requestType, arduinoLintLibraryManagerSetting, submissionURLs = parseDiff(diff, "repositories.txt")
requestType, requestError, arduinoLintLibraryManagerSetting, submissionURLs = parseDiff(diff, "repositories.txt")
assert.Equal(t, "removal", requestType, testName)
assert.Equal(t, "", requestError, testName)
assert.Equal(t, "", arduinoLintLibraryManagerSetting, testName)
assert.Nil(t, submissionURLs, testName)

Expand All @@ -159,8 +166,9 @@ index cff484d..8b401a1 100644
+https://github.com/foo/bar
`)

requestType, arduinoLintLibraryManagerSetting, submissionURLs = parseDiff(diff, "repositories.txt")
requestType, requestError, arduinoLintLibraryManagerSetting, submissionURLs = parseDiff(diff, "repositories.txt")
assert.Equal(t, "modification", requestType, testName)
assert.Equal(t, "", requestError, testName)
assert.Equal(t, "update", arduinoLintLibraryManagerSetting, testName)
assert.Equal(t, submissionURLs, []string{"https://github.com/foo/bar"}, testName)
}
Expand Down
40 changes: 34 additions & 6 deletions test/test_all.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,30 @@


@pytest.mark.parametrize(
"repopath_folder_name, expected_type, expected_submissions, expected_indexentry, expected_indexerlogsurls",
"repopath_folder_name,"
"expected_type,"
"expected_error,"
"expected_submissions,"
"expected_indexentry,"
"expected_indexerlogsurls",
[
("list-deleted-diff", "other", None, "", ""),
("multi-file-diff", "other", None, "", ""),
("non-list-diff", "other", None, "", ""),
("list-rename-diff", "other", None, "", ""),
("removal", "removal", None, "", ""),
("list-deleted-diff", "other", "", None, "", ""),
("multi-file-diff", "other", "", None, "", ""),
("non-list-diff", "other", "", None, "", ""),
("list-rename-diff", "other", "", None, "", ""),
(
"no-final-newline-diff",
"invalid",
"Pull request removes newline from the end of a file.%0APlease add a blank line to the end of the file.",
None,
"",
"",
),
("removal", "removal", "", None, "", ""),
(
"modification",
"modification",
"",
[
{
"submissionURL": "https://github.com/arduino-libraries/ArduinoCloudThing",
Expand All @@ -48,6 +62,7 @@
(
"url-error",
"submission",
"",
[
{
"submissionURL": "foo",
Expand All @@ -65,6 +80,7 @@
(
"url-404",
"submission",
"",
[
{
"submissionURL": "http://httpstat.us/404",
Expand All @@ -82,6 +98,7 @@
(
"not-supported-git-host",
"submission",
"",
[
{
"submissionURL": "https://example.com",
Expand All @@ -101,6 +118,7 @@
(
"not-git-clone-url",
"submission",
"",
[
{
"submissionURL": "https://github.com/arduino-libraries/ArduinoCloudThing/releases",
Expand All @@ -119,6 +137,7 @@
(
"already-in-library-manager",
"submission",
"",
[
{
"submissionURL": "https://github.com/arduino-libraries/Servo",
Expand All @@ -136,6 +155,7 @@
(
"type-arduino",
"submission",
"",
[
{
"submissionURL": "https://github.com/arduino-libraries/ArduinoCloudThing",
Expand All @@ -153,6 +173,7 @@
(
"type-partner",
"submission",
"",
[
{
"submissionURL": "https://github.com/ms-iot/virtual-shields-arduino",
Expand All @@ -170,6 +191,7 @@
(
"type-recommended",
"submission",
"",
[
{
"submissionURL": "https://github.com/adafruit/Adafruit_TinyFlash",
Expand All @@ -187,6 +209,7 @@
(
"type-contributed",
"submission",
"",
[
{
"submissionURL": "https://github.com/sparkfun/SparkFun_Ublox_Arduino_Library",
Expand All @@ -205,6 +228,7 @@
(
"no-tags",
"submission",
"",
[
{
"submissionURL": "https://github.com/arduino/cloud-examples",
Expand All @@ -224,6 +248,7 @@
(
"no-library-properties",
"submission",
"",
[
{
"submissionURL": "https://github.com/arduino-libraries/WiFiLink-Firmware",
Expand All @@ -242,6 +267,7 @@
(
"duplicates-in-submission",
"submission",
"",
[
{
"submissionURL": "https://github.com/arduino-libraries/ArduinoCloudThing",
Expand Down Expand Up @@ -273,6 +299,7 @@ def test_request(
run_command,
repopath_folder_name,
expected_type,
expected_error,
expected_submissions,
expected_indexentry,
expected_indexerlogsurls,
Expand All @@ -286,6 +313,7 @@ def test_request(

request = json.loads(result.stdout)
assert request["type"] == expected_type
assert request["error"] == expected_error
assert request["submissions"] == expected_submissions
assert request["indexEntry"] == expected_indexentry
assert request["submissions"] == expected_submissions
Expand Down
9 changes: 9 additions & 0 deletions test/testdata/no-final-newline-diff/diff.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
diff --git a/repositories.txt b/repositories.txt
index c080a7a..dcba162 100644
--- a/repositories.txt
+++ b/repositories.txt
@@ -1,2 +1,3 @@
https://github.com/arduino-libraries/Servo
https://github.com/arduino-libraries/Stepper
+https://github.com/arduino-libraries/ArduinoCloudThing
\ No newline at end of file
2 changes: 2 additions & 0 deletions test/testdata/no-final-newline-diff/repositories.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
https://github.com/arduino-libraries/Servo
https://github.com/arduino-libraries/Stepper

0 comments on commit 0e0f23e

Please sign in to comment.