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

Implement a count-offenses command #340

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
+47 −15
Draft

Implement a count-offenses command #340

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
df580ea
Remove unused instance variable from initializer
davidstosik Mar 28, 2023
a93824d
Remove orphan entry in USAGE.md TOC
davidstosik Mar 29, 2023
d1ac0ea
Refactor ParseRun#find_offenses
davidstosik Mar 30, 2023
15cfab5
Implement `packwerk show-offenses` command
davidstosik Mar 30, 2023
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
1 change: 0 additions & 1 deletion USAGE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
* [Understanding how to respond to new violations](#understanding-how-to-respond-to-new-violations)
* [Recording existing violations](#recording-existing-violations)
* [Understanding the package todo file](#understanding-the-package-todo-file)
* [Understanding the list of deprecated references](#understanding-the-list-of-deprecated-references)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This points to a non-existent entry in the doc.

* [Loading extensions](#loading-extensions)

## What problem does Packwerk solve?
Expand Down
3 changes: 3 additions & 0 deletions lib/packwerk/cli.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@ def execute_command(args)
output_result(parse_run(args).check)
when "update-todo", "update"
output_result(parse_run(args).update_todo)
when "show-offenses", "show"
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if violations or offense is the right wording here. We should be more consistent with out wording internally. Since we've already gotten things like "offense formatters", this is probably fine.

Copy link
Contributor Author

@davidstosik davidstosik Mar 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I agree it would be nice to define the gem's vocabulary more precisely. It would not only help with understanding how to use it, but also make reading/writing code easier.

Copy link
Contributor

Choose a reason for hiding this comment

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

Big plus for this! We call these three things:

  • todos (e.g. update-todo, package_todo.yml
  • violations
  • offenses

IMO violations and offenses are somewhat aggressive terms and TODO captures the desire to change/fix while being less aggressive and also shorter to write!

I'd be in support of changing all language across the board to todos! Some things could be changed right away (e.g. internal implementation variable names), others could probably be changed while not being considered a public API breakage (e.g. output of commands), others would need to be deprecated or have a switchover before a major version change (e.g. CLI commands, offenses formatter, the word violation in package_todo files).

What do you all think? (We could start a discussion too.)

Copy link
Contributor Author

@davidstosik davidstosik Apr 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure I agree with using the word TODO for all.
To me, the word todo in Packwerk has the same meaning as in Rubocop's .rubocop_todo.yml, which is there to disable offenses "to be fixed later":

The generated file .rubocop_todo.yml contains configuration to disable cops that currently detect an offense in the code by changing the configuration for the cop, excluding the offending files, or disabling the cop altogether once a file count limit has been reached. (source)

I think we could use the same word for violations and offenses, but would not conflate that concept with todo.
Offense feels a bit more neutral to me (and I guess it also matches Rubocop's vocabulary). The word is also already used more than twice more than violation:

ag -i offense | wc -l
     799
ag -i violation | wc -l
     325

I have a hunch that there might have been the intent to give violation and offense slightly different meanings, but 1. I'm not sure I'm right and 2. this might not be necessary, or could be expressed more explicitly:

  • offense: anything that's causing an error (config error, reference that violates declared dependencies, etc)
  • violation: an offense that is not recorded as a todo (and that triggers an error on check).

Copy link
Contributor

Choose a reason for hiding this comment

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

Interestingly, even though offense is used more often in the source code, violation (at least at Gusto) is used much more often by the client. Not sure if it's the same at Shopify, but here everyone uses the word violation, and it's probably in part because the word violation is in package_todo.yml files explicitly. Also, the output of runs uses the word violation (e.g. "no stale violations detected"), although the word offenses is used too sometimes.

I can buy the different use cases for the words. Something is an offense for example in packwerk if there's a file that can't be parsed correctly. A violation is an offense, but is also what happens when everything works correctly in packwerk, but there's a "violation" of packwerk directives (e.g. dependency usage, private API usage, etc.). TODOs are recorded violations, perhaps short for "violations TODO."

I'm not sure if all of this distinction is useful to the client using this system though, but also I haven't really heard of many folks confused about these terms besides us so perhaps it's not a problem! I'm just glad we replaced deprecated_references with todo though – I've found that has resolved a lot of language confusion on our end.

output_result(parse_run(args).show_offenses)
when "validate"
validate(args)
when "version"
Expand Down Expand Up @@ -118,6 +120,7 @@ def usage
Subcommands:
init - set up packwerk
check - run all checks
show-offenses - displays all offenses
update-todo - update package_todo.yml files
validate - verify integrity of packwerk and package configuration
version - output packwerk version
Expand Down
5 changes: 5 additions & 0 deletions lib/packwerk/offense_collection.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,11 @@ def listed?(offense)
package_todo_for(reference.package).listed?(reference, violation_type: offense.violation_type)
end

sig { params(offenses: T::Array[Offense]).void }
def add_offenses(offenses)
offenses.each { |offense| add_offense(offense) }
end

sig do
params(offense: Packwerk::Offense).void
end
Expand Down
1 change: 0 additions & 1 deletion lib/packwerk/package.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ def initialize(name:, config: nil)
@name = name
@config = T.let(config || {}, T::Hash[String, T.untyped])
@dependencies = T.let(Array(@config["dependencies"]).freeze, T::Array[String])
@public_path = T.let(nil, T.nilable(String))
Copy link
Contributor Author

@davidstosik davidstosik Mar 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is never used.

end

sig { returns(T::Boolean) }
Expand Down
52 changes: 39 additions & 13 deletions lib/packwerk/parse_run.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,10 @@ def update_todo
end

run_context = RunContext.from_configuration(@configuration)
offense_collection = find_offenses(run_context)
offenses = find_offenses(run_context) { update_progress }

offense_collection = OffenseCollection.new(@configuration.root_path)
offense_collection.add_offenses(offenses)
offense_collection.persist_package_todo_files(run_context.package_set)

message = <<~EOS
Expand All @@ -57,10 +60,25 @@ def update_todo
Cli::Result.new(message: message, status: offense_collection.errors.empty?)
end

sig { returns(Cli::Result) }
def show_offenses
run_context = RunContext.from_configuration(@configuration)
all_offenses = find_offenses(run_context)

message = @offenses_formatter.show_offenses(all_offenses)

Cli::Result.new(message: message, status: true)
gmcgibbon marked this conversation as resolved.
Show resolved Hide resolved
end

sig { returns(Cli::Result) }
def check
run_context = RunContext.from_configuration(@configuration)
offense_collection = find_offenses(run_context, show_errors: true)
offense_collection = OffenseCollection.new(@configuration.root_path)
offenses = find_offenses(run_context) do |offenses|
failed = offenses.any? { |offense| !offense_collection.listed?(offense) }
update_progress(failed: failed)
end
offense_collection.add_offenses(offenses)

messages = [
@offenses_formatter.show_offenses(offense_collection.outstanding_offenses),
Expand All @@ -76,16 +94,25 @@ def check

private

sig { params(run_context: RunContext, show_errors: T::Boolean).returns(OffenseCollection) }
def find_offenses(run_context, show_errors: false)
offense_collection = OffenseCollection.new(@configuration.root_path)
sig do
params(
run_context: RunContext,
block: T.nilable(T.proc.params(
offenses: T::Array[Packwerk::Offense],
).void)
).returns(T::Array[Offense])
end
def find_offenses(run_context, &block)
all_offenses = T.let([], T::Array[Offense])
process_file = T.let(->(relative_file) do
run_context.process_file(relative_file: relative_file).tap do |offenses|
failed = show_errors && offenses.any? { |offense| !offense_collection.listed?(offense) }
update_progress(failed: failed)
end
end, ProcessFileProc)
process_file = if block
T.let(proc do |relative_file|
run_context.process_file(relative_file: relative_file).tap(&block)
end, ProcessFileProc)
else
T.let(proc do |relative_file|
run_context.process_file(relative_file: relative_file)
end, ProcessFileProc)
end
Copy link
Member

Choose a reason for hiding this comment

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

Can we move this to a private method? This is slightly repetitive, but I think it is justified with how many times this gets called.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean something like this?

def process_file_proc(&block)
  if block
    T.let(proc do |relative_file|
      run_context.process_file(relative_file: relative_file).tap(&block)
    end, ProcessFileProc)
  else
    T.let(proc do |relative_file|
      run_context.process_file(relative_file: relative_file)
    end, ProcessFileProc)
  end
end

# Then I can call it where it's used:
# ...
      @progress_formatter.started_inspection(@relative_file_set) do
        all_offenses = if @configuration.parallel?
          Parallel.flat_map(@relative_file_set, &process_file_proc(&block))
        else
          serial_find_offenses(&process_file_proc(&block))
        end
      end
# ...
# (Or I can keep a local variable if that seems better.)


@progress_formatter.started_inspection(@relative_file_set) do
all_offenses = if @configuration.parallel?
Expand All @@ -95,8 +122,7 @@ def find_offenses(run_context, show_errors: false)
end
end

all_offenses.each { |offense| offense_collection.add_offense(offense) }
offense_collection
all_offenses
end

sig { params(block: ProcessFileProc).returns(T::Array[Offense]) }
Expand Down