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

Added Toggle .only package #6734

Closed
wants to merge 3 commits into from
Closed

Conversation

CurtisHumphrey
Copy link

Toggles ".only" on and off of describe and it for test files in javascript

Copy link
Collaborator

@packagecontrol-bot packagecontrol-bot left a comment

Choose a reason for hiding this comment

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

Automated testing result: SUCCESS

Repo link: Toggle .only

Packages added:
  - Toggle .only

Processing package "Toggle .only"
  - All checks passed

@FichteFoll
Copy link
Collaborator

You cannot use dots in the package's name due to how Python importing works.

@CurtisHumphrey
Copy link
Author

Are you sure @FichteFoll as I have this installed via add github source and everything has been working fine. I think the name field was just a pretty print thing.

@FichteFoll
Copy link
Collaborator

Yes, I am sure. #2524 covers this.

Package Control installs the package under the name specified, i.e. it either calls your package Toggle .only.sublime-package or extracts it to a folder named Toggle .only (if .no-sublime-package is provided, which isn't necessary here). You can try this yourself, as importing the Python plugin will fail.

@CurtisHumphrey
Copy link
Author

hmm, I see what you are getting at now. They way I installed it, it use the repo name. Fixing it in just a moment.

Copy link
Collaborator

@packagecontrol-bot packagecontrol-bot left a comment

Choose a reason for hiding this comment

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

Automated testing result: SUCCESS

Repo link: Toggle only for Tests

Packages added:
  - Toggle only for Tests

Processing package "Toggle only for Tests"
  - All checks passed

@FichteFoll
Copy link
Collaborator

You'll have to fix the package order after the name change: https://travis-ci.org/wbond/package_control_channel/builds/308559511#L675

Copy link
Collaborator

@packagecontrol-bot packagecontrol-bot left a comment

Choose a reason for hiding this comment

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

Automated testing result: SUCCESS

Repo link: Toggle only for Tests

Packages added:
  - Toggle only for Tests

Processing package "Toggle only for Tests"
  - All checks passed

@Thom1729
Copy link
Collaborator

Thom1729 commented Jan 3, 2018

I read through the documentation and I still can't quite tell what this package is for. Some specific unit-testing framework? What framework? Is there an existing package for that framework that may provide similar functionality, or to which this functionality could be added?

You're creating a global keybinding that applies in every scope. If possible, you should add context conditions to the binding so that it's only available when you're using the relevant testing framework.

You're only performing the modification on the first selection. You should probably do it on every selection. (This would also handle the case where there is no selection.)

@FichteFoll
Copy link
Collaborator

I agree with these.

You're creating a global keybinding that applies in every scope. If possible, you should add context conditions to the binding so that it's only available when you're using the relevant testing framework.

You're only performing the modification on the first selection. You should probably do it on every selection. (This would also handle the case where there is no selection.)

Please address this feedback before we proceed with the merge.
I don't care which testing framework this is for, but I don't recall having seen a package like this before.

@CurtisHumphrey
Copy link
Author

So If I understand correctly you would like:

  • Better readme file that says what framework this is for
  • Somehow add context conditions to restrict it to only certain type of files?
  • Allow modification on more than one sections

I did quite a bit of looking but the other packages (ones related to Mocha or Jasmine - both JS testing) seem to all be related to either running the tests or generating templates. This one doesn't really fit with either as it more a helper for toggle tests on and off that are auto-running in the background somewhere on a file watcher. I'm not sure how to do the context but I can look it up.

@FichteFoll
Copy link
Collaborator

  • Somehow add context conditions to restrict it to only certain type of files?
  • Allow modification on more than one sections

These are the important parts here. If you need any help with that, I can guide you towards documentation or an implementation.

@FichteFoll
Copy link
Collaborator

@CurtisHumphrey ping

@FichteFoll FichteFoll added the stale The pull request needs to be updated but has not been within the recent past (2 weeks) label Feb 19, 2018
@FichteFoll FichteFoll added timeout A pull request needed changes but was not updated in time (2 weeks after becoming stale) and removed stale The pull request needs to be updated but has not been within the recent past (2 weeks) labels Mar 15, 2018
@FichteFoll FichteFoll closed this Mar 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
timeout A pull request needed changes but was not updated in time (2 weeks after becoming stale)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants