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

Fix Thor warning due to missing :after option #252

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Inkybro
Copy link

@Inkybro Inkybro commented Jul 22, 2020

Hey, I was trying to mess around a little with Sorcery tonight on Rails 6.0.3.2, Ruby 2.7.1p83, and I kept noticing some odd warning message about a flag not being set when I tried to use the generators. I am pretty sure it was actually harmless, and my "fixes" here may ultimately not be necessary or desirable, but I was bored and wanted to see if I could get rid of the warning.

I tried with 0.15.0, as well as the master branch, and still get the warning: File unchanged! The supplied flag value not found! app/models/user.rb (see more below)

rails g sorcery:install
Running via Spring preloader in process 2257
      create  config/initializers/sorcery.rb
    generate  model User --skip-migrationrails g sorcery:install
Running via Spring preloader in process 2257
      create  config/initializers/sorcery.rb
    generate  model User --skip-migration
       rails  generate model User --skip-migration 
Running via Spring preloader in process 2318
      invoke  active_record
      create    app/models/user.rb
      invoke    test_unit
      create      test/models/user_test.rb
      create      test/fixtures/users.yml
      insert  app/models/user.rb
File unchanged! The supplied flag value not found!  app/models/user.rb
      create  db/migrate/20200722041629_sorcery_core.rb
       rails  generate model User --skip-migration 
Running via Spring preloader in process 2318
      invoke  active_record
      create    app/models/user.rb
      invoke    test_unit
      create      test/models/user_test.rb
      create      test/fixtures/users.yml
      insert  app/models/user.rb
File unchanged! The supplied flag value not found!  app/models/user.rb
      create  db/migrate/20200722041629_sorcery_core.rb

It is worth mentioning that the line still did get added to the model, despite the misleading warning. I ended up tracing the warning message back to Thor, and it looks like it's expecting an :after option to let it know where to insert the content.

I also accidentally entered my model name without the --model flag while testing the generator a few times and it was slightly annoying, so I also added the list of available submodules into the generator with a little validation and guidance for users. Other than that I just did a little re-arranging/re-factoring.

I really wasn't sure how one would go about writing specs for a generator. If anyone would like to point me in the right direction, I'd be willing to give it a shot. It looks like none of this code was really tested in the first place, unless I just totally missed it, and looks like the code hadn't been touched too much in a while. I did run the specs before I opened this PR, though.

Anyway, I was just bored. Feel free to throw this away if it's useless or way off base or anything.

@joshbuker
Copy link
Member

It looks like none of this code was really tested in the first place, unless I just totally missed it, and looks like the code hadn't been touched too much in a while.

I don't think you missed any specs, for the most part there haven't been any. Adding tests to all functionality is a major part of the Sorcery v1 rework.

I really wasn't sure how one would go about writing specs for a generator. If anyone would like to point me in the right direction, I'd be willing to give it a shot.

I'm not really sure either yet. Would you be interested in doing some research to see if any other libraries have specs for their generators? Rails might be a good candidate for that. I'm still in the process of grokking the crypto and config, so it'll be a while before I get to reworking the generators for v1.

@Inkybro
Copy link
Author

Inkybro commented Jul 22, 2020

I will try and find some time to do that :) Thanks for the feedback!

@Inkybro
Copy link
Author

Inkybro commented Jul 27, 2020

At an absolute stand-still for now. Any help is welcome.

@Inkybro
Copy link
Author

Inkybro commented Jul 27, 2020

My fellow wizards.. Please forgive me. I got so close.

Ultimately I am too frustrated. I have to chill for a minute.

If the solution is simple here, please do point it out.

@joshbuker
Copy link
Member

It's still going to be a bit before I'm to the generators unfortunately. I recently finished the Rails bootstrapping part (god that was a hurdle), but there's still the config refactor which will be pretty hefty, as well as some other less major refactoring.

@Inkybro how about taking a break from this for now, and once I'm to the generators I can reach out to you and we can look at this together?

@Inkybro
Copy link
Author

Inkybro commented Jul 27, 2020

@athix Even better, why don't you let me help you with something else? I'm happy to revisit this -- I just know when to cut my losses.

@joshbuker
Copy link
Member

@Inkybro Something that would be helpful is consolidating all the open issues and PRs, along with the old stuff from #32 into essentially one big wishlist of changes. It may save some time if those considerations are made up-front rather than after the refactor is complete, and as it is right now I'm knee deep in trying to grok the existing code.

@Inkybro
Copy link
Author

Inkybro commented Jul 27, 2020

got u, bb

@Inkybro
Copy link
Author

Inkybro commented Jul 27, 2020

Somehow I accidentally "liked" my own thing?

Ok, no problem. I will do that but tonight I will relax because i worked all week!!

@joshbuker
Copy link
Member

Sounds good, thanks @Inkybro! Hope you have a relaxing afternoon. 👍

@Inkybro
Copy link
Author

Inkybro commented Jul 27, 2020

get some rest my friend

@Inkybro Inkybro force-pushed the fix-missing-after-flag-for-thor branch from a3d9a9d to 4a49ffe Compare August 7, 2020 14:53
@Inkybro
Copy link
Author

Inkybro commented Aug 7, 2020

@athix I finally got some very basic generator specs working.

Would love to hear your feedback, let me know what you think.

Going to try and find some time next week to work on the other things we discussed earlier in this thread.

@joshbuker joshbuker added the to be implemented in v1 This issue or pull request will be resolved in the v1 rework, but has not yet been completed. label May 28, 2021
@joshbuker
Copy link
Member

I'm going to have to do some heavy lifting generator-wise for v1 regardless, so I'll just focus on adding generator tests and getting it all fixed up in v1 instead of trying to muddle through the v0 stuff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
to be implemented in v1 This issue or pull request will be resolved in the v1 rework, but has not yet been completed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants