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.

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

generator-for-devise-jwt: Implement the generator for devise-jwt #96

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

TheZero0-ctrl
Copy link
Contributor

Task done

  • Implement the generator for devise-jwt

@TheZero0-ctrl
Copy link
Contributor Author

@abhaynikam Could you please review this PR at your convenience and let me know if any changes are necessary? Thank you for your time.

@abhaynikam
Copy link
Owner

Update: I will review this tonight.

@coolprobn
Copy link
Collaborator

@TheZero0-ctrl Let's fix merge conflicts here

Copy link
Owner

@abhaynikam abhaynikam left a comment

Choose a reason for hiding this comment

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

Left some comments. Excited to get this merged 🥇


def verify_presence_of_devise_gem
gem_file_content_array = File.readlines("Gemfile")
devise_is_installed = gem_file_content_array.any? { |line| line.include?('devise') }
Copy link
Owner

Choose a reason for hiding this comment

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

I think I recently added a helper method called gem_installed?. can you try using that here?

abort
end

def verify_presence_of_devise_model
Copy link
Owner

Choose a reason for hiding this comment

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

just my preference, I usually use ensure_ for such checks. fine to keep this if you prefer this way :)


def add_devise_jwt_gem
say "Adding devise-jwt gem", :green
gem "devise-jwt"
Copy link
Owner

Choose a reason for hiding this comment

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

will this run bundle installer?


def configure_jti_matcher_strategy
model_name = options[:model_name].underscore
Bundler.with_unbundled_env do
Copy link
Owner

Choose a reason for hiding this comment

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

can you use migration template here instead? I think that would be easier in long run?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I find it challenging to determine the next migration number for the migration file name. Do you have any suggestions for a better approach? I am considering using the current timestamp.

@abhaynikam

Copy link
Contributor Author

@TheZero0-ctrl TheZero0-ctrl May 24, 2024

Choose a reason for hiding this comment

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

I find this function on rails source code

        def self.next_migration_number(dirname)
          next_migration_number = current_migration_number(dirname) + 1
          ActiveRecord::Migration.next_migration_number(next_migration_number)
        end

can I use this and use migration_template method by including Rails::Generators::Migration ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

here is the documentation of migration_template

Creates a migration template at the given destination. The difference to the default template method is that the migration version is appended to the destination file name.

The migration version, migration file name, migration class name are available as instance variables in the template to be rendered.


end

def configure_denylist_strategy
Copy link
Owner

Choose a reason for hiding this comment

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

Ditto. I think easier to just add templates for all migrations you want for denylist.

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.

3 participants