Skip to content

Allow packs to opt-out directories from automatic namespacing#13

Merged
gap777 merged 2 commits intoFlytedesk:mainfrom
jordanbyron:jtb/feat-exclude-dir-override
Mar 6, 2023
Merged

Allow packs to opt-out directories from automatic namespacing#13
gap777 merged 2 commits intoFlytedesk:mainfrom
jordanbyron:jtb/feat-exclude-dir-override

Conversation

@jordanbyron
Copy link
Contributor

@jordanbyron jordanbyron commented Mar 2, 2023

Small feature which allows individual packs to excluding additional directories from automatic namespacing.

  • chore: Refactor non_namspaced_directory with specs
  • feat: Add automatic_pack_namespace_exclusions config for packs

Extract from the updated README:

Additional directories can be excluded by adding them to the automatic_pack_namespace_exclusions key in your package.yml file:

metadata:
  automatic_pack_namespace: true
  automatic_pack_namespace_exclusions:
    - app/policies

namespaced_packages.each do |pack, metadata|
package_namespace = define_namespace(pack, metadata)
pack_directories(pack.path).each do |pack_dir|
pack_directories(pack.path, metadata).each do |pack_dir|
Copy link
Contributor Author

@jordanbyron jordanbyron Mar 2, 2023

Choose a reason for hiding this comment

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

@gap777 this is probably the smelliest part of my change (passing down the metadata variable). I'm game to fix that up, but I didn't want to hit you with a big refactor to review without chatting with you first. Thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

What about something like this?

 def pack_directories(pack_root_dir, metadata)
    Dir.glob("#{pack_root_dir}/**/app/*").select { |dir| namspaced_directory?(dir, metadata) }
  end

  def namespaced_directory?(dir, metadata)
    !excluded_directories(metadata).include?(dir)
  end
  
  def excluded_directories(metadata)
    DEFAULT_EXCLUDED_DIRS + metadata.fetch(PACKAGE_EXCLUDED_DIRS_KEY, [])
  end

Copy link
Contributor Author

@jordanbyron jordanbyron Mar 2, 2023

Choose a reason for hiding this comment

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

Updated, but I did have to change the namespaced_directory? method so the logic worked as intended.

irb(main):008:0> excluded_directories = %w[/app/helpers /app/excluded]
=> ["/app/helpers", "/app/excluded"]
irb(main):009:0> dir = "/spec/fixtures/rails-7.0/packs/hats/summer/app/excluded"
=> "/spec/fixtures/rails-7.0/packs/hats/summer/app/excluded"
irb(main):010:0> !excluded_directories.include?(dir) # Expect to be false
=> true
irb(main):011:0> excluded_directories.none? { |excluded_dir| dir.include?(excluded_dir) } # Expect to be false
=> false

This configuration allows packs to opt-out specific directories from
automatic namespacing.
@jordanbyron jordanbyron force-pushed the jtb/feat-exclude-dir-override branch from bc599a9 to eb0ce80 Compare March 2, 2023 19:04
@jordanbyron jordanbyron requested a review from gap777 March 2, 2023 19:10
excluded_directories(metadata).none? { |excluded_dir| dir.include?(excluded_dir) }
end

def excluded_directories(metadata)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see the problem.. thanks for addressing the logic error. It appears that it stems from a naming confusion, in that these are all called 'dirs' but really, some are relatively named paths and some are absolutely named paths. We could probably improve the code quite a bit by naming things better, and possibly even harmonizing to use the same kind of thing in both cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could probably improve the code quite a bit by naming things better, and possibly even harmonizing to use the same kind of thing in both cases.

Totally agree, and thank you again for taking the time to review my changes. Is this something you'd like me to take a pass at before we merge this PR in?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's go with this for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's go with this for now.

Awesome. Let me know if there is anything I can do to get this merged and released. We've got a few teams hoping to leverage this feature soon.

@gap777 gap777 merged commit 4732d35 into Flytedesk:main Mar 6, 2023
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.

2 participants