Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Update FLUTTER_NOLINT uses to include issue link #21921

Merged
cbracken merged 1 commit into from
Oct 16, 2020
Merged

Update FLUTTER_NOLINT uses to include issue link #21921

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions shell/platform/linux/fl_json_message_codec.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,6 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

// TODO(cbracken): lint disabled due to rapidjson null deref issue.
// https://github.com/flutter/flutter/issues/65676
// FLUTTER_NOLINT

#include "flutter/shell/platform/linux/public/flutter_linux/fl_json_message_codec.h"

#include <gmodule.h>
Expand Down
3 changes: 2 additions & 1 deletion shell/version/version.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
// FLUTTER_NOLINT

// FLUTTER_NOLINT: https://github.com/flutter/flutter/issues/68332
Copy link
Member

@chinmaygarde chinmaygarde Oct 16, 2020

Choose a reason for hiding this comment

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

Can we just use one meta-issue with a checklist for these?

Copy link
Member Author

@cbracken cbracken Oct 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Not entirely opposed to it but I'm concerned that that has a lot of the same issues as not having a link at all; people are tempted to just copy-paste without investigating further. This would also increase the likelihood of adding the NOLINT but forgetting to add it to the bug.

Part of the goal here is to make it painful to add new opt-outs by being forced to file a bug that accurately describes the issue. I think it's fine to link to an existing bug for cases where it's the same fundamental issue though. But in that case, the incentive is still there for the filer/reviewer to check that the issue is the right one.

Copy link
Member Author

@cbracken cbracken Oct 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

After this PR, we're down to two in the entire codebase. #21927 gets us down to a single remaining issue, so there won't be much to track on a meta-issue either way.

If I get time, I'll look into killing off the last one today or next week.


#include "flutter/shell/version/version.h"

Expand Down
3 changes: 2 additions & 1 deletion vulkan/vulkan_window.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
// FLUTTER_NOLINT

// FLUTTER_NOLINT: https://github.com/flutter/flutter/issues/68331

#include "vulkan_window.h"

Expand Down