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

Add an option to control recursive search & parsing of source files. #69

Closed
perrette opened this issue Jun 19, 2015 · 10 comments
Closed

Comments

@perrette
Copy link

As we started to discuss in issue #61 , FoBiS.py could be made more robust by enhancing user control (without losing the automated features that make FoBiS.py attractive in the first place). An easy step would be to include a directories= parameter. For the cumbersome example of the README, it would look like:

directories = ["src/", "src/nested-1", "src/nested-1/nested-2"] 

(this is for the fobos config file, but there would be an equivalent command-line parameter --directories src src/nested-1 src/nested-1/nested-2). This parameter would indicate FoBiS.py which directories to search for source files, in a non-recursive way.

@szaghi suggested that, if the only effect of a directories= parameter would be to limit the recursive search to certain directories, the parameter could be named only=. I think however this would be ambiguous, and an independent specification of directories= and recursive= is preferable. For example, recursive=True and directories=["src1", "src2"] is a perfectly valid use case, useful when only src1 and src2 sub-directories of the current location should be searched for and parsed (out of the possibly many directories present here (e.g. "obj", "src1-old", "other-project"), but still in a recursive way. On the other hand, the above cumbersome example with directories= specification would be associated with recursive=False.

I would make the further strong suggestion that FoBiS.py becomes non-recursive by default, unless an explicit -r or --recursive flag is passed (recursive=True in fobos), like most unix commands that actually process file contents (e.g. grep, cp, ctags). Adding a -r is really a minor additional burden for the user, and is much more compatible with the unix command-line philosophy.

As a summary:

  • the current behavior would be obtained by passing a -r or --recursive flag (equivalently, recursive = True in fobos file, by default, False)
  • if none of --recursive or --directories is specified, only files in the current folder are searched for or parsed (i.e. default is recursive=False and directories=["./"]).
  • the preferred behavior in a fobos config file would be to specify directories=.
@perrette
Copy link
Author

An additional argument to have recursive=False by default is to offer the best compromise for the various use cases:

  • old behavior : FoBiS.py build -r
  • new, preferred behavior: FoBiS.py build --directories src1 src1/lib1

Otherwise, you need to add another flag --non-recursive to be added to the second line, which makes the syntax really awkward FoBiS.py build --directories src1 src1/lib1 --non-recursive and de facto discourages its use. Make the default value of the --recursive option dependent of what --directories would also be awkward. I think there is no way around it.

@perrette
Copy link
Author

Of course, in the command-line--directories could also be passed as -d, it is not yet taken I believe.

@jacobwilliams
Copy link

I also like the idea of having a non-recursive option. I lean toward thinking the current (recursive) behavior should be the default...but would be OK with it either way.

@Tobychev
Copy link
Contributor

I think the we could name the variable "onlyin", it is semantically clearer and does not take a position regarding how deep down the hierarchy you go.

I side with @szaghi on the issue of defaults, and as far as I can see this is largely an argument about personal preferences. The reference to the default behaviour of the core file manipulation programs of unix is not very relevant because FoBiS is not a program dedicated to interacting with the file system but rather to abstract away the details of the file system.

@perrette
Copy link
Author

@Tobychev
I need to disagree: a recursive search of files is performed, and these ALL need to be parsed in the first place (unless --target is specified), thus de facto it interacts with the file system. Additionally, with --onlyin you do not solve the problem of having independent parameters for a) the list of root source directories and b) whether the search is recursive or not.

@Tobychev
Copy link
Contributor

My suggestion was to rename --directory into --onlyin because I think the latter more clearly conveys that only certain directories will be visited. I did not imply any other change with that suggestion..

@perrette
Copy link
Author

Ok, I was assuming that having an --only parameter implied that the search would become non-recursive (and therefore was an alternative suggestion to have both --directories (or any name) and --recursive).

This is a matter of preference indeed. Personally, I find --directories more explicit, but maybe the opinion of a native English speaker would be more useful.

By the way, I realize there is already a -s SRC, --src SRC option, so the simplest would be to just extend it to accept a list : -s SRC [SRC ...], --src SRC [SRC ...]. Then either add a --non-recursive flag, or change the default behavior and add a -r, --recursive flag (still my preferred way, according to my experience of trying to avoid surprises as much as possible, with higher priority compared to trying to automate everything)

@szaghi
Copy link
Owner

szaghi commented Nov 9, 2015

Dear All,

I am sorry of my huge delay, I am very busy.

I see the points of all of you and I greatly appreciate your help, thank you very much!

I am going to implement these features... however, I will start with a low-profile approach.

Firstly, I agree to add a non recursive mode, but I prefer to maintain the default behavior to be recursive: I think a lot on the points of @perrette and I agree on almost all of them (I like to be more posix as possible...), however my soul go to Fortran poor people like me. We (I hope) appreciate to lunch simply a build to have all programs compiled and requiring to lunch build -r will break some of the magic I love of FoBiS. Therefore, the first step I am going to do is to a switch to disable the recursive search. After this we can talk about the mechanism to specify a pool of target. Besides, I decided to modify the --exclude switch in order to allow it to accept also directories (currently only files should be supported).

Stay tuned!

P.S. the doctests feature is here :-)

@szaghi
Copy link
Owner

szaghi commented Nov 11, 2015

In the v1.8.2 just uploaded I have added a new option:

--disable_recursive_search/-drs that does what we said... the recursive search is disabled. The default behavior remains to be recursive. I have added a specific unit tests (built-test16) where it is proven that with this option only the first level programs are build, the recursion is disabled.

No I will work on how to support directories/pool of targets.

Your opinions on this new option are very appreciated.

@szaghi
Copy link
Owner

szaghi commented Nov 16, 2015

@perrette @jacobwilliams @Tobychev and all,

I finally end up with this solution (quick and dirty):

  • add --disable_recursive_search/-drs option: as said this option disable the recursive search for parsed files in the case you want to disable it; by default the search is recursive as before;
  • modify --src/-s option: now it accepts also list of directories not only one; this allows you to organize your project with more flexibility of the directories tree;
  • add -build_all option: this allows you to build all parsed files (programs, modules and old-fashioned non-module-container files).

All of these options (as well as the others) can be mixed. Therefore I think that many of your corners usages should be covered. For the @jacobwilliams issue#70 there is still a little step to do: with -build_all option now you can build all modules with only one command, but if you add -mklib static/shared you will obtain a library .a/.so for each module, thus you have to glue them manually. I am thinking how to trigger the creation of only one library in this specific case.

I am closing this issue and others, but feel free to replay with your critics and suggestions and I will reopen them if necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants