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

feat: add --ignore property/flag #98

Closed
wants to merge 5 commits into from
Closed

feat: add --ignore property/flag #98

wants to merge 5 commits into from

Conversation

danielpza
Copy link
Contributor

Supports .gitignore style

@codecov
Copy link

codecov bot commented Mar 13, 2020

Codecov Report

Merging #98 into develop will decrease coverage by 0.72%.
The diff coverage is 66.66%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop      #98      +/-   ##
===========================================
- Coverage    94.65%   93.92%   -0.73%     
===========================================
  Files           20       19       -1     
  Lines          486      477       -9     
  Branches       113      115       +2     
===========================================
- Hits           460      448      -12     
- Misses          26       29       +3
Impacted Files Coverage Δ
src/index/getFilePaths.ts 100% <100%> (ø) ⬆️
src/index.ts 66.66% <25%> (-3.93%) ⬇️
src/index/printHelpMessage.ts

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c7130dd...06b3503. Read the comment docs.

@AnatoleLucet
Copy link
Collaborator

Why using ignore instead of glob?

@danielpza
Copy link
Contributor Author

No reason, wasn't aware of the glob options, I'll update accordingly.

@AnatoleLucet
Copy link
Collaborator

Nice. Also are you disposed to add tests cases for this feature?

@danielpza
Copy link
Contributor Author

I'm playing around with some of my projects with destiny to see how it goes, I'll probably do it later.

@AnatoleLucet
Copy link
Collaborator

Great! No problem.

README.md Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
danielpza and others added 3 commits March 15, 2020 14:48
Co-Authored-By: Anatole Lucet <[email protected]>
Co-Authored-By: Anatole Lucet <[email protected]>
Co-Authored-By: Anatole Lucet <[email protected]>
@danielpza
Copy link
Contributor Author

danielpza commented Mar 15, 2020

Hi @AnatoleLucet, just to let you know I haven't forgotten about the PR.
I found a case where I didn't like the results, iirc I had to specify a glob instead of a folder (eg --ignore lib -> --ignore 'lib/*')

@AnatoleLucet
Copy link
Collaborator

AnatoleLucet commented Mar 15, 2020

just to let you know I haven't forgotten about the PR.

No problem, there is no urgency

I found a case where I didn't like the results, iirc I had to specify a glob instead of a folder (eg --ignore lib -> --ignore 'lib/*')

I'm not sure what you mean.
Do you mean --ignore lib doesn't work, but --ignore 'lib/*' does?

@danielpza
Copy link
Contributor Author

danielpza commented Mar 15, 2020

Do you mean --ignore lib doesn't work, but --ignore 'lib/*' does?

yes, it doesn't work irrc, or something like that

@AnatoleLucet
Copy link
Collaborator

AnatoleLucet commented Mar 23, 2020

Hey @danielpza, so I've looked a bit deeper on how this feature could be implemented.
Ignoring files in the getRestructureMap module is not enought. Destiny should also fix imports from / of ignored files.
For example if we take the index-cycle fixture, and we try to ignore the home folder (destiny . --ignore home), it shouldn't move the home folder but probably fix his imports.
You should look into the generateTrees and fixImports modules.

Also, my requested change in parseArgs seems to be wrong, the following should fix it:

  while (args.length > 0) {
    const arg = args.shift();

    if (arg == null) break;

+    let nextOptionIdx = args.findIndex(x => x.startsWith("-"));
+    nextOptionIdx = nextOptionIdx === -1 ? args.length : nextOptionIdx;

    switch (arg) {
      case "-h":
      case "--help":
        cliConfig.help = true;
        break;
      case "-V":
      case "--version":
        cliConfig.version = true;
        break;
      case "-w":
      case "--write":
        cliConfig.write = true;
        break;
      case "-S":
      case "--avoid-single-file":
        cliConfig.avoidSingleFile = true;
        break;
      case "--ignore":
-        const nextOptionIdx = args.findIndex(x => x.startsWith("-"));
-
        cliConfig.ignore = [...(cliConfig.ignore ?? []), ...(args.splice(0, nextOptionIdx) ?? [])];
        break;
      default: {
        if (fs.existsSync(arg) || glob.hasMagic(arg)) {
          cliConfig.include = [...(cliConfig.include ?? []), arg];
        }
      }
    }

@lifeiscontent
Copy link

@danielpza would love to see this feature wrapped up, destiny is currently crawling my node_modules folder 🗡️

@danielpza
Copy link
Contributor Author

danielpza commented May 15, 2020

just to let you know I haven't forgotten about the PR

Well, I totally forgot about the PR, but I also kinda lost interest on it, you are free to pick up from where I left or just make a new one, that will probably be easier.

I usually have a root folder ./src where I put my code, so the node_modules case it's not an issue for me.

@danielpza danielpza deleted the ignore-flag branch May 16, 2020 18:54
@danielpza danielpza restored the ignore-flag branch May 16, 2020 18:55
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